[llvm] ebdadcf - [SPIR-V] Improve correctness of emitted MIR between passes for branching instructions (#106966)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 3 10:02:09 PDT 2024
Author: Vyacheslav Levytskyy
Date: 2024-09-03T19:02:03+02:00
New Revision: ebdadcfeb9c63f56703bf47dfeb6aff5a66ddfa1
URL: https://github.com/llvm/llvm-project/commit/ebdadcfeb9c63f56703bf47dfeb6aff5a66ddfa1
DIFF: https://github.com/llvm/llvm-project/commit/ebdadcfeb9c63f56703bf47dfeb6aff5a66ddfa1.diff
LOG: [SPIR-V] Improve correctness of emitted MIR between passes for branching instructions (#106966)
This PR improves correctness of emitted MIR between passes for branching
instructions and thus increase number of passing tests when expensive
checks are on. Specifically, we address here such issues with machine
verifier as:
* fix switch generation: generate correct successors and undo the
"address taken" status to reflect that a successor doesn't actually
correspond to an IR-level basic block;
* fix incorrect definition of OpBranch and OpBranchConditional in
TableGen (SPIRVInstrInfo.td) to set isBarrier status properly and set a
correct type of virtual registers;
* fix a case when Phi refers to a type definition that goes after the
Phi instruction, so that the virtual register definition of the type
doesn't dominate all uses.
This PR decrease number of failing tests under expensive checks from 56
to 50.
Added:
Modified:
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
llvm/test/CodeGen/SPIRV/branching/OpSwitchBranches.ll
llvm/test/CodeGen/SPIRV/branching/OpSwitchEmpty.ll
llvm/test/CodeGen/SPIRV/branching/OpSwitchUnreachable.ll
llvm/test/CodeGen/SPIRV/branching/Two_OpSwitch_same_register.ll
llvm/test/CodeGen/SPIRV/transcoding/GlobalFunAnnotate.ll
Removed:
################################################################################
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
index 54bcb96772e7ab..553d86efa3df34 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
@@ -168,8 +168,6 @@ SPIRVGlobalRegistry::getOrCreateConstIntReg(uint64_t Val, SPIRVType *SpvType,
ConstantInt *CI = ConstantInt::get(const_cast<IntegerType *>(LLVMIntTy), Val);
Register Res = DT.find(CI, CurMF);
if (!Res.isValid()) {
- // TODO: handle cases where the type is not 32bit wide
- // TODO: https://github.com/llvm/llvm-project/issues/88129
Res =
CurMF->getRegInfo().createGenericVirtualRegister(LLT::scalar(BitWidth));
CurMF->getRegInfo().setRegClass(Res, &SPIRV::iIDRegClass);
@@ -197,8 +195,6 @@ SPIRVGlobalRegistry::getOrCreateConstFloatReg(APFloat Val, SPIRVType *SpvType,
auto *const CI = ConstantFP::get(Ctx, Val);
Register Res = DT.find(CI, CurMF);
if (!Res.isValid()) {
- // TODO: handle cases where the type is not 32bit wide
- // TODO: https://github.com/llvm/llvm-project/issues/88129
Res =
CurMF->getRegInfo().createGenericVirtualRegister(LLT::scalar(BitWidth));
CurMF->getRegInfo().setRegClass(Res, &SPIRV::fIDRegClass);
@@ -391,8 +387,6 @@ Register SPIRVGlobalRegistry::getOrCreateCompositeOrNull(
SpvScalConst =
getOrCreateBaseRegister(Val, I, SpvType, TII, BitWidth, ZeroAsNull);
- // TODO: handle cases where the type is not 32bit wide
- // TODO: https://github.com/llvm/llvm-project/issues/88129
LLT LLTy = LLT::scalar(64);
Register SpvVecConst =
CurMF->getRegInfo().createGenericVirtualRegister(LLTy);
diff --git a/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp b/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
index 8db9808bb87e1d..682fca7cc7747c 100644
--- a/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
@@ -353,6 +353,7 @@ void SPIRVTargetLowering::finalizeLowering(MachineFunction &MF) const {
GR.setCurrentFunc(MF);
for (MachineFunction::iterator I = MF.begin(), E = MF.end(); I != E; ++I) {
MachineBasicBlock *MBB = &*I;
+ SmallPtrSet<MachineInstr *, 8> ToMove;
for (MachineBasicBlock::iterator MBBI = MBB->begin(), MBBE = MBB->end();
MBBI != MBBE;) {
MachineInstr &MI = *MBBI++;
@@ -456,8 +457,23 @@ void SPIRVTargetLowering::finalizeLowering(MachineFunction &MF) const {
MI.removeOperand(i);
}
} break;
+ case SPIRV::OpPhi: {
+ // Phi refers to a type definition that goes after the Phi
+ // instruction, so that the virtual register definition of the type
+ // doesn't dominate all uses. Let's place the type definition
+ // instruction at the end of the predecessor.
+ MachineBasicBlock *Curr = MI.getParent();
+ SPIRVType *Type = GR.getSPIRVTypeForVReg(MI.getOperand(1).getReg());
+ if (Type->getParent() == Curr && !Curr->pred_empty())
+ ToMove.insert(const_cast<MachineInstr *>(Type));
+ } break;
}
}
+ for (MachineInstr *MI : ToMove) {
+ MachineBasicBlock *Curr = MI->getParent();
+ MachineBasicBlock *Pred = *Curr->pred_begin();
+ Pred->insert(Pred->getFirstTerminator(), Curr->remove_instr(MI));
+ }
}
ProcessedMF.insert(&MF);
TargetLowering::finalizeLowering(MF);
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
index 79b9bb87739fec..0cbd2f4536075d 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
+++ b/llvm/lib/Target/SPIRV/SPIRVInstrInfo.td
@@ -622,9 +622,11 @@ def OpLoopMerge: Op<246, (outs), (ins ID:$merge, ID:$continue, LoopControl:$lc,
def OpSelectionMerge: Op<247, (outs), (ins ID:$merge, SelectionControl:$sc),
"OpSelectionMerge $merge $sc">;
def OpLabel: Op<248, (outs ID:$label), (ins), "$label = OpLabel">;
+let isBarrier = 1, isTerminator=1 in {
+ def OpBranch: Op<249, (outs), (ins unknown:$label), "OpBranch $label">;
+}
let isTerminator=1 in {
- def OpBranch: Op<249, (outs), (ins ID:$label), "OpBranch $label">;
- def OpBranchConditional: Op<250, (outs), (ins ID:$cond, ID:$true, ID:$false, variable_ops),
+ def OpBranchConditional: Op<250, (outs), (ins ID:$cond, unknown:$true, unknown:$false, variable_ops),
"OpBranchConditional $cond $true $false">;
def OpSwitch: Op<251, (outs), (ins ID:$sel, ID:$dflt, variable_ops), "OpSwitch $sel $dflt">;
}
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
index 4e45aa4b888a92..df1b75bc1cb9eb 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
@@ -778,8 +778,10 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
}
SmallPtrSet<MachineInstr *, 8> ToEraseMI;
+ SmallPtrSet<MachineBasicBlock *, 8> ClearAddressTaken;
for (auto &SwIt : Switches) {
MachineInstr &MI = *SwIt.first;
+ MachineBasicBlock *MBB = MI.getParent();
SmallVector<MachineInstr *, 8> &Ins = SwIt.second;
SmallVector<MachineOperand, 8> NewOps;
for (unsigned i = 0; i < Ins.size(); ++i) {
@@ -790,8 +792,11 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
if (It == BB2MBB.end())
report_fatal_error("cannot find a machine basic block by a basic "
"block in a switch statement");
- NewOps.push_back(MachineOperand::CreateMBB(It->second));
- MI.getParent()->addSuccessor(It->second);
+ MachineBasicBlock *Succ = It->second;
+ ClearAddressTaken.insert(Succ);
+ NewOps.push_back(MachineOperand::CreateMBB(Succ));
+ if (!llvm::is_contained(MBB->successors(), Succ))
+ MBB->addSuccessor(Succ);
ToEraseMI.insert(Ins[i]);
} else {
NewOps.push_back(
@@ -830,6 +835,12 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
}
BlockAddrI->eraseFromParent();
}
+
+ // BlockAddress operands were used to keep information between passes,
+ // let's undo the "address taken" status to reflect that Succ doesn't
+ // actually correspond to an IR-level basic block.
+ for (MachineBasicBlock *Succ : ClearAddressTaken)
+ Succ->setAddressTakenIRBlock(nullptr);
}
static bool isImplicitFallthrough(MachineBasicBlock &MBB) {
diff --git a/llvm/test/CodeGen/SPIRV/branching/OpSwitchBranches.ll b/llvm/test/CodeGen/SPIRV/branching/OpSwitchBranches.ll
index 454b51d952a365..f5dc1efa2fb957 100644
--- a/llvm/test/CodeGen/SPIRV/branching/OpSwitchBranches.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/OpSwitchBranches.ll
@@ -1,4 +1,8 @@
-; RUN: llc -O0 -mtriple=spirv32-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 %}
+
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
define i32 @test_switch_branches(i32 %a) {
entry:
diff --git a/llvm/test/CodeGen/SPIRV/branching/OpSwitchEmpty.ll b/llvm/test/CodeGen/SPIRV/branching/OpSwitchEmpty.ll
index c309883577405b..768ab6ed2a05c2 100644
--- a/llvm/test/CodeGen/SPIRV/branching/OpSwitchEmpty.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/OpSwitchEmpty.ll
@@ -8,7 +8,11 @@
;; Command:
;; clang -cc1 -triple spir -emit-llvm -o test/SPIRV/OpSwitchEmpty.ll OpSwitchEmpty.cl -disable-llvm-passes
-; RUN: llc -O0 -mtriple=spirv32-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 %}
+
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; CHECK-SPIRV: %[[#X:]] = OpFunctionParameter %[[#]]
; CHECK-SPIRV: OpSwitch %[[#X]] %[[#DEFAULT:]]{{$}}
diff --git a/llvm/test/CodeGen/SPIRV/branching/OpSwitchUnreachable.ll b/llvm/test/CodeGen/SPIRV/branching/OpSwitchUnreachable.ll
index 6eb36e5756ecf6..7fbd06c67b56e0 100644
--- a/llvm/test/CodeGen/SPIRV/branching/OpSwitchUnreachable.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/OpSwitchUnreachable.ll
@@ -1,4 +1,7 @@
-; RUN: llc -O0 -mtriple=spirv32-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 %}
+
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
define void @test_switch_with_unreachable_block(i1 %a) {
diff --git a/llvm/test/CodeGen/SPIRV/branching/Two_OpSwitch_same_register.ll b/llvm/test/CodeGen/SPIRV/branching/Two_OpSwitch_same_register.ll
index f5c12e1c0f5a3f..265226cbfe2380 100644
--- a/llvm/test/CodeGen/SPIRV/branching/Two_OpSwitch_same_register.ll
+++ b/llvm/test/CodeGen/SPIRV/branching/Two_OpSwitch_same_register.ll
@@ -1,6 +1,9 @@
-; 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 %}
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
+; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
+
define spir_kernel void @test_two_switch_same_register(i32 %value) {
; CHECK-SPIRV: OpSwitch %[[#REGISTER:]] %[[#DEFAULT1:]] 1 %[[#CASE1:]] 0 %[[#CASE2:]]
switch i32 %value, label %default1 [
diff --git a/llvm/test/CodeGen/SPIRV/transcoding/GlobalFunAnnotate.ll b/llvm/test/CodeGen/SPIRV/transcoding/GlobalFunAnnotate.ll
index 33bece5b9c00f7..d17e228c2ef88d 100644
--- a/llvm/test/CodeGen/SPIRV/transcoding/GlobalFunAnnotate.ll
+++ b/llvm/test/CodeGen/SPIRV/transcoding/GlobalFunAnnotate.ll
@@ -1,4 +1,4 @@
-; RUN: llc -O0 -mtriple=spirv64-unknown-linux %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
+; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-linux %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
; TODO: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
; CHECK-SPIRV: OpDecorate %[[#]] UserSemantic "annotation_on_function"
More information about the llvm-commits
mailing list