[SPIR-V] Improve general validity of emitted code between passes #119202
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-spir-v Author: Vyacheslav Levytskyy (VyacheslavLevytskyy) ChangesThis PR improves general validity of emitted code between passes due to generation of The test suite with expensive checks set ON now has only 18 fails out of 569 total test cases. Full diff: https://github.com/llvm/llvm-project/pull/119202.diff 14 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
index fabedb5e06c1d5..fb9efbdceafacb 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
@@ -1551,14 +1551,17 @@ SPIRVType *SPIRVGlobalRegistry::getOrCreateSPIRVPointerType(
if (Reg.isValid())
return getSPIRVTypeForVReg(Reg);
// create a new type
- auto MIB = BuildMI(MIRBuilder.getMBB(), MIRBuilder.getInsertPt(),
- MIRBuilder.getDebugLoc(),
- MIRBuilder.getTII().get(SPIRV::OpTypePointer))
- .addDef(createTypeVReg(CurMF->getRegInfo()))
- .addImm(static_cast<uint32_t>(SC))
- .addUse(getSPIRVTypeID(BaseType));
- DT.add(PointerElementType, AddressSpace, CurMF, getSPIRVTypeID(MIB));
- return finishCreatingSPIRVType(LLVMTy, MIB);
+ return createOpType(MIRBuilder, [&](MachineIRBuilder &MIRBuilder) {
+ auto MIB = BuildMI(MIRBuilder.getMBB(), MIRBuilder.getInsertPt(),
+ MIRBuilder.getDebugLoc(),
+ MIRBuilder.getTII().get(SPIRV::OpTypePointer))
+ .addDef(createTypeVReg(CurMF->getRegInfo()))
+ .addImm(static_cast<uint32_t>(SC))
+ .addUse(getSPIRVTypeID(BaseType));
+ DT.add(PointerElementType, AddressSpace, CurMF, getSPIRVTypeID(MIB));
+ finishCreatingSPIRVType(LLVMTy, MIB);
+ return MIB;
+ });
}
SPIRVType *SPIRVGlobalRegistry::getOrCreateSPIRVPointerType(
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index 3a98b74b3d6757..98edf53e721320 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -2162,6 +2162,7 @@ bool SPIRVInstructionSelector::selectBuildVector(Register ResVReg,
report_fatal_error(
"There must be at least two constituent operands in a vector");
+ MRI->setRegClass(ResVReg, GR.getRegClass(ResType));
auto MIB = BuildMI(*I.getParent(), I, I.getDebugLoc(),
TII.get(IsConst ? SPIRV::OpConstantComposite
: SPIRV::OpCompositeConstruct))
@@ -2195,6 +2196,7 @@ bool SPIRVInstructionSelector::selectSplatVector(Register ResVReg,
report_fatal_error(
"There must be at least two constituent operands in a vector");
+ MRI->setRegClass(ResVReg, GR.getRegClass(ResType));
auto MIB = BuildMI(*I.getParent(), I, I.getDebugLoc(),
TII.get(IsConst ? SPIRV::OpConstantComposite
: SPIRV::OpCompositeConstruct))
@@ -2701,7 +2703,7 @@ bool SPIRVInstructionSelector::wrapIntoSpecConstantOp(
continue;
}
// Create a new register for the wrapper
- WrapReg = MRI->createVirtualRegister(&SPIRV::iIDRegClass);
+ WrapReg = MRI->createVirtualRegister(GR.getRegClass(OpType));
GR.add(OpDefine, MF, WrapReg);
CompositeArgs.push_back(WrapReg);
// Decorate the wrapper register and generate a new instruction
@@ -2766,6 +2768,7 @@ bool SPIRVInstructionSelector::selectIntrinsic(Register ResVReg,
if (!wrapIntoSpecConstantOp(I, CompositeArgs))
return false;
}
+ MRI->setRegClass(ResVReg, GR.getRegClass(ResType));
auto MIB = BuildMI(BB, I, I.getDebugLoc(), TII.get(Opcode))
.addDef(ResVReg)
.addUse(GR.getSPIRVTypeID(ResType));
@@ -3388,7 +3391,10 @@ bool SPIRVInstructionSelector::selectPhi(Register ResVReg,
MIB.addUse(I.getOperand(i + 0).getReg());
MIB.addMBB(I.getOperand(i + 1).getMBB());
}
- return MIB.constrainAllUses(TII, TRI, RBI);
+ bool Res = MIB.constrainAllUses(TII, TRI, RBI);
+ MIB->setDesc(TII.get(TargetOpcode::PHI));
+ MIB->removeOperand(1);
+ return Res;
}
bool SPIRVInstructionSelector::selectGlobalValue(
diff --git a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
index 4ee71c703f90bf..4dea4056799fec 100644
--- a/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVModuleAnalysis.cpp
@@ -1770,6 +1770,27 @@ static void addMBBNames(const Module &M, const SPIRVInstrInfo &TII,
}
}
+// patching Instruction::PHI to SPIRV::OpPhi
+static void patchPhis(const Module &M, SPIRVGlobalRegistry *GR,
+ const SPIRVInstrInfo &TII, MachineModuleInfo *MMI) {
+ for (auto F = M.begin(), E = M.end(); F != E; ++F) {
+ MachineFunction *MF = MMI->getMachineFunction(*F);
+ if (!MF)
+ continue;
+ for (auto &MBB : *MF) {
+ for (MachineInstr &MI : MBB) {
+ if (MI.getOpcode() != TargetOpcode::PHI)
+ continue;
+ MI.setDesc(TII.get(SPIRV::OpPhi));
+ Register ResTypeReg = GR->getSPIRVTypeID(
+ GR->getSPIRVTypeForVReg(MI.getOperand(0).getReg(), MF));
+ MI.insert(MI.operands_begin() + 1,
+ {MachineOperand::CreateReg(ResTypeReg, false)});
+ }
+ }
+ }
+}
+
struct SPIRV::ModuleAnalysisInfo SPIRVModuleAnalysis::MAI;
void SPIRVModuleAnalysis::getAnalysisUsage(AnalysisUsage &AU) const {
@@ -1788,6 +1809,8 @@ bool SPIRVModuleAnalysis::runOnModule(Module &M) {
setBaseInfo(M);
+ patchPhis(M, GR, *TII, MMI);
+
addMBBNames(M, *TII, MMI, *ST, MAI);
addDecorations(M, *TII, MMI, *ST, MAI);
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
index d5299043d9ef2f..ceccf55d1de4df 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
@@ -466,8 +466,13 @@ void processInstr(MachineInstr &MI, MachineIRBuilder &MIB,
for (auto &Op : MI.operands()) {
if (!Op.isReg() || Op.isDef())
continue;
- auto IdOpInfo = createNewIdReg(nullptr, Op.getReg(), MRI, *GR);
- MIB.buildInstr(IdOpInfo.second).addDef(IdOpInfo.first).addUse(Op.getReg());
+ Register OpReg = Op.getReg();
+ SPIRVType *SpvType = GR->getSPIRVTypeForVReg(OpReg);
+ auto IdOpInfo = createNewIdReg(SpvType, OpReg, MRI, *GR);
+ MIB.buildInstr(IdOpInfo.second).addDef(IdOpInfo.first).addUse(OpReg);
+ const TargetRegisterClass *RC = GR->getRegClass(SpvType);
+ if (RC != MRI.getRegClassOrNull(OpReg))
+ MRI.setRegClass(OpReg, RC);
Op.setReg(IdOpInfo.first);
}
}
diff --git a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
index 8f38d4b8307da2..d9d0d00cc71e41 100644
--- a/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVTargetMachine.cpp
@@ -130,13 +130,8 @@ FunctionPass *SPIRVPassConfig::createTargetRegisterAllocator(bool) {
return nullptr;
}
-// Disable passes that may break CFG.
+// A place to disable passes that may break CFG.
void SPIRVPassConfig::addMachineSSAOptimization() {
- // Some standard passes that optimize machine instructions in SSA form uses
- // MI.isPHI() that doesn't account for OpPhi in SPIR-V and so are able to
- // break the CFG (e.g., MachineSink).
- disablePass(&MachineSinkingID);
-
TargetPassConfig::addMachineSSAOptimization();
}
diff --git a/llvm/test/CodeGen/SPIRV/branching/if-merging.ll b/llvm/test/CodeGen/SPIRV/branching/if-merging.ll
index eb8ca515c58225..18fbe417addf8c 100644
--- a/llvm/test/CodeGen/SPIRV/branching/if-merging.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/if-merging.ll
@@ -1,4 +1,4 @@
-; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
;; NOTE: This does not check for structured control-flow operations.
diff --git a/llvm/test/CodeGen/SPIRV/branching/if-non-merging.ll b/llvm/test/CodeGen/SPIRV/branching/if-non-merging.ll
index f978c44888e0f3..b664a65a5829c8 100644
--- a/llvm/test/CodeGen/SPIRV/branching/if-non-merging.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/if-non-merging.ll
@@ -1,4 +1,4 @@
-; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
; CHECK-DAG: [[I32:%.+]] = OpTypeInt 32
; CHECK-DAG: [[BOOL:%.+]] = OpTypeBool
diff --git a/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll b/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll
index 9981e0d86eaa34..25c2c8627de4e6 100644
--- a/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/switch-range-check.ll
@@ -1,4 +1,4 @@
-; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; CHECK: OpFunction
diff --git a/llvm/test/CodeGen/SPIRV/half_no_extension.ll b/llvm/test/CodeGen/SPIRV/half_no_extension.ll
index a5b0ec9c92d236..63cecbc7399aec 100644
--- a/llvm/test/CodeGen/SPIRV/half_no_extension.ll
+++ b/llvm/test/CodeGen/SPIRV/half_no_extension.ll
@@ -5,10 +5,11 @@
;; vstorea_half4_rtp( data, 0, f );
;; }
-; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
-; CHECK-SPIRV: OpCapability Float16Buffer
-; CHECK-SPIRV-NOT: OpCapability Float16
+; CHECK-DAG: OpCapability Float16Buffer
+; CHECK-DAG: OpCapability Float16
define spir_kernel void @test(<4 x float> addrspace(1)* %p, half addrspace(1)* %f) {
entry:
diff --git a/llvm/test/CodeGen/SPIRV/keep-tracked-const.ll b/llvm/test/CodeGen/SPIRV/keep-tracked-const.ll
index 0c25d8a67aca05..0dc86233f8e160 100644
--- a/llvm/test/CodeGen/SPIRV/keep-tracked-const.ll
+++ b/llvm/test/CodeGen/SPIRV/keep-tracked-const.ll
@@ -1,6 +1,6 @@
; This test case ensures that cleaning of temporary constants doesn't purge tracked ones.
-; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; CHECK-SPIRV: %[[#Int:]] = OpTypeInt 32 0
diff --git a/llvm/test/CodeGen/SPIRV/phi-insert-point.ll b/llvm/test/CodeGen/SPIRV/phi-insert-point.ll
index 41cc514f4dbae3..70d121cdf4b3ac 100644
--- a/llvm/test/CodeGen/SPIRV/phi-insert-point.ll
+++ b/llvm/test/CodeGen/SPIRV/phi-insert-point.ll
@@ -2,7 +2,7 @@
; operand are inserted at the correct positions, and don't break rules of
; instruction domination and PHI nodes grouping at top of basic block.
-; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; CHECK-DAG: OpName %[[#Foo:]] "foo"
diff --git a/llvm/test/CodeGen/SPIRV/phi-ptrcast-dominate.ll b/llvm/test/CodeGen/SPIRV/phi-ptrcast-dominate.ll
index 102eb76356fe32..bc090ce55fbecd 100644
--- a/llvm/test/CodeGen/SPIRV/phi-ptrcast-dominate.ll
+++ b/llvm/test/CodeGen/SPIRV/phi-ptrcast-dominate.ll
@@ -3,7 +3,7 @@
; positions, and don't break rules of instruction domination and PHI nodes
; grouping at top of basic block.
-; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
diff --git a/llvm/test/CodeGen/SPIRV/phi-spvintrinsic-dominate.ll b/llvm/test/CodeGen/SPIRV/phi-spvintrinsic-dominate.ll
index 471ab03ed89f65..45831ce97263d2 100644
--- a/llvm/test/CodeGen/SPIRV/phi-spvintrinsic-dominate.ll
+++ b/llvm/test/CodeGen/SPIRV/phi-spvintrinsic-dominate.ll
@@ -3,7 +3,7 @@
; positions, and don't break rules of instruction domination and PHI nodes
; grouping at top of basic block.
-; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; CHECK: OpFunction
diff --git a/llvm/test/CodeGen/SPIRV/transcoding/OpPhi_ArgumentsPlaceholders.ll b/llvm/test/CodeGen/SPIRV/transcoding/OpPhi_ArgumentsPlaceholders.ll
index ee5596ed38b1b7..63b517c8054942 100644
--- a/llvm/test/CodeGen/SPIRV/transcoding/OpPhi_ArgumentsPlaceholders.ll
+++ b/llvm/test/CodeGen/SPIRV/transcoding/OpPhi_ArgumentsPlaceholders.ll
@@ -14,7 +14,6 @@
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
-; XFAIL: *
%struct.Node = type { %struct.Node.0 addrspace(1)* }
%struct.Node.0 = type opaque
|
michalpaszkowski
approved these changes
Dec 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves general validity of emitted code between passes due to generation of
TargetOpcode::PHIinstead ofSPIRV::OpPhiafter Instruction Selection, fixing generation of OpTypePointer instructions and using of proper virtual register classes.Using
TargetOpcode::PHIinstead ofSPIRV::OpPhiafter Instruction Selection has a benefit to support existing optimization passes immediately, as an alternative path to disable those passes that useMI.isPHI(). This PR makes it possible thus to revert #116060 actions and get back to use theMachineSinkpass.This PR is a solution of the problem discussed in details in #110507. It accepts an advice from code reviewers of the PR #110507 to postpone generation of OpPhi rather than to patch CodeGen. This solution allows to unblock improvements wrt. expensive checks and makes it unrelated to the general points of the discussion about OpPhi vs. G_PHI/PHI.
This PR contains numerous small patches of emitted code validity that allows to substantially pass rate with expensive checks. Namely, the test suite with expensive checks set ON now has only 12 fails out of 569 total test cases.
FYI @bogner