Skip to content

Commit ebdadcf

Browse files
VyacheslavLevytskyyVyacheslavLevytskyy
authored
[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.
1 parent a8e1c6f commit ebdadcf

9 files changed

+52
-15
lines changed

‎llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp

-6
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,6 @@ SPIRVGlobalRegistry::getOrCreateConstIntReg(uint64_t Val, SPIRVType *SpvType,
168168
ConstantInt *CI = ConstantInt::get(const_cast<IntegerType *>(LLVMIntTy), Val);
169169
Register Res = DT.find(CI, CurMF);
170170
if (!Res.isValid()) {
171-
// TODO: handle cases where the type is not 32bit wide
172-
// TODO: https://github.com/llvm/llvm-project/issues/88129
173171
Res =
174172
CurMF->getRegInfo().createGenericVirtualRegister(LLT::scalar(BitWidth));
175173
CurMF->getRegInfo().setRegClass(Res, &SPIRV::iIDRegClass);
@@ -197,8 +195,6 @@ SPIRVGlobalRegistry::getOrCreateConstFloatReg(APFloat Val, SPIRVType *SpvType,
197195
auto *const CI = ConstantFP::get(Ctx, Val);
198196
Register Res = DT.find(CI, CurMF);
199197
if (!Res.isValid()) {
200-
// TODO: handle cases where the type is not 32bit wide
201-
// TODO: https://github.com/llvm/llvm-project/issues/88129
202198
Res =
203199
CurMF->getRegInfo().createGenericVirtualRegister(LLT::scalar(BitWidth));
204200
CurMF->getRegInfo().setRegClass(Res, &SPIRV::fIDRegClass);
@@ -391,8 +387,6 @@ Register SPIRVGlobalRegistry::getOrCreateCompositeOrNull(
391387
SpvScalConst =
392388
getOrCreateBaseRegister(Val, I, SpvType, TII, BitWidth, ZeroAsNull);
393389

394-
// TODO: handle cases where the type is not 32bit wide
395-
// TODO: https://github.com/llvm/llvm-project/issues/88129
396390
LLT LLTy = LLT::scalar(64);
397391
Register SpvVecConst =
398392
CurMF->getRegInfo().createGenericVirtualRegister(LLTy);

‎llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp

+16
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,7 @@ void SPIRVTargetLowering::finalizeLowering(MachineFunction &MF) const {
353353
GR.setCurrentFunc(MF);
354354
for (MachineFunction::iterator I = MF.begin(), E = MF.end(); I != E; ++I) {
355355
MachineBasicBlock *MBB = &*I;
356+
SmallPtrSet<MachineInstr *, 8> ToMove;
356357
for (MachineBasicBlock::iterator MBBI = MBB->begin(), MBBE = MBB->end();
357358
MBBI != MBBE;) {
358359
MachineInstr &MI = *MBBI++;
@@ -456,8 +457,23 @@ void SPIRVTargetLowering::finalizeLowering(MachineFunction &MF) const {
456457
MI.removeOperand(i);
457458
}
458459
} break;
460+
case SPIRV::OpPhi: {
461+
// Phi refers to a type definition that goes after the Phi
462+
// instruction, so that the virtual register definition of the type
463+
// doesn't dominate all uses. Let's place the type definition
464+
// instruction at the end of the predecessor.
465+
MachineBasicBlock *Curr = MI.getParent();
466+
SPIRVType *Type = GR.getSPIRVTypeForVReg(MI.getOperand(1).getReg());
467+
if (Type->getParent() == Curr && !Curr->pred_empty())
468+
ToMove.insert(const_cast<MachineInstr *>(Type));
469+
} break;
459470
}
460471
}
472+
for (MachineInstr *MI : ToMove) {
473+
MachineBasicBlock *Curr = MI->getParent();
474+
MachineBasicBlock *Pred = *Curr->pred_begin();
475+
Pred->insert(Pred->getFirstTerminator(), Curr->remove_instr(MI));
476+
}
461477
}
462478
ProcessedMF.insert(&MF);
463479
TargetLowering::finalizeLowering(MF);

‎llvm/lib/Target/SPIRV/SPIRVInstrInfo.td

+4-2
Original file line numberDiff line numberDiff line change
@@ -622,9 +622,11 @@ def OpLoopMerge: Op<246, (outs), (ins ID:$merge, ID:$continue, LoopControl:$lc,
622622
def OpSelectionMerge: Op<247, (outs), (ins ID:$merge, SelectionControl:$sc),
623623
"OpSelectionMerge $merge $sc">;
624624
def OpLabel: Op<248, (outs ID:$label), (ins), "$label = OpLabel">;
625+
let isBarrier = 1, isTerminator=1 in {
626+
def OpBranch: Op<249, (outs), (ins unknown:$label), "OpBranch $label">;
627+
}
625628
let isTerminator=1 in {
626-
def OpBranch: Op<249, (outs), (ins ID:$label), "OpBranch $label">;
627-
def OpBranchConditional: Op<250, (outs), (ins ID:$cond, ID:$true, ID:$false, variable_ops),
629+
def OpBranchConditional: Op<250, (outs), (ins ID:$cond, unknown:$true, unknown:$false, variable_ops),
628630
"OpBranchConditional $cond $true $false">;
629631
def OpSwitch: Op<251, (outs), (ins ID:$sel, ID:$dflt, variable_ops), "OpSwitch $sel $dflt">;
630632
}

‎llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp

+13-2
Original file line numberDiff line numberDiff line change
@@ -778,8 +778,10 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
778778
}
779779

780780
SmallPtrSet<MachineInstr *, 8> ToEraseMI;
781+
SmallPtrSet<MachineBasicBlock *, 8> ClearAddressTaken;
781782
for (auto &SwIt : Switches) {
782783
MachineInstr &MI = *SwIt.first;
784+
MachineBasicBlock *MBB = MI.getParent();
783785
SmallVector<MachineInstr *, 8> &Ins = SwIt.second;
784786
SmallVector<MachineOperand, 8> NewOps;
785787
for (unsigned i = 0; i < Ins.size(); ++i) {
@@ -790,8 +792,11 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
790792
if (It == BB2MBB.end())
791793
report_fatal_error("cannot find a machine basic block by a basic "
792794
"block in a switch statement");
793-
NewOps.push_back(MachineOperand::CreateMBB(It->second));
794-
MI.getParent()->addSuccessor(It->second);
795+
MachineBasicBlock *Succ = It->second;
796+
ClearAddressTaken.insert(Succ);
797+
NewOps.push_back(MachineOperand::CreateMBB(Succ));
798+
if (!llvm::is_contained(MBB->successors(), Succ))
799+
MBB->addSuccessor(Succ);
795800
ToEraseMI.insert(Ins[i]);
796801
} else {
797802
NewOps.push_back(
@@ -830,6 +835,12 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
830835
}
831836
BlockAddrI->eraseFromParent();
832837
}
838+
839+
// BlockAddress operands were used to keep information between passes,
840+
// let's undo the "address taken" status to reflect that Succ doesn't
841+
// actually correspond to an IR-level basic block.
842+
for (MachineBasicBlock *Succ : ClearAddressTaken)
843+
Succ->setAddressTakenIRBlock(nullptr);
833844
}
834845

835846
static bool isImplicitFallthrough(MachineBasicBlock &MBB) {

‎llvm/test/CodeGen/SPIRV/branching/OpSwitchBranches.ll

+5-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,8 @@
1-
; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
1+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
2+
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
3+
4+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
5+
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
26

37
define i32 @test_switch_branches(i32 %a) {
48
entry:

‎llvm/test/CodeGen/SPIRV/branching/OpSwitchEmpty.ll

+5-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@
88
;; Command:
99
;; clang -cc1 -triple spir -emit-llvm -o test/SPIRV/OpSwitchEmpty.ll OpSwitchEmpty.cl -disable-llvm-passes
1010

11-
; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
11+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
12+
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
13+
14+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
15+
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
1216

1317
; CHECK-SPIRV: %[[#X:]] = OpFunctionParameter %[[#]]
1418
; CHECK-SPIRV: OpSwitch %[[#X]] %[[#DEFAULT:]]{{$}}

‎llvm/test/CodeGen/SPIRV/branching/OpSwitchUnreachable.ll

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,7 @@
1-
; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
1+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
2+
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
3+
4+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
25
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
36

47
define void @test_switch_with_unreachable_block(i1 %a) {

‎llvm/test/CodeGen/SPIRV/branching/Two_OpSwitch_same_register.ll

+4-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
1-
; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
1+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
22
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
33

4+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
5+
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv32-unknown-unknown %s -o - -filetype=obj | spirv-val %}
6+
47
define spir_kernel void @test_two_switch_same_register(i32 %value) {
58
; CHECK-SPIRV: OpSwitch %[[#REGISTER:]] %[[#DEFAULT1:]] 1 %[[#CASE1:]] 0 %[[#CASE2:]]
69
switch i32 %value, label %default1 [

‎llvm/test/CodeGen/SPIRV/transcoding/GlobalFunAnnotate.ll

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
; RUN: llc -O0 -mtriple=spirv64-unknown-linux %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
1+
; RUN: llc -verify-machineinstrs -O0 -mtriple=spirv64-unknown-linux %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
22
; TODO: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
33

44
; CHECK-SPIRV: OpDecorate %[[#]] UserSemantic "annotation_on_function"

0 commit comments

Comments
 (0)