[llvm] [SPIR-V] Improve correctness of emitted MIR between passes for branching instructions (PR #106966)
Vyacheslav Levytskyy via llvm-commits
llvm-commits at lists.llvm.org
Mon Sep 2 05:15:33 PDT 2024
https://github.com/VyacheslavLevytskyy updated https://github.com/llvm/llvm-project/pull/106966
>From 67f933038ff279c9135635fb5f6bd81b851f52f0 Mon Sep 17 00:00:00 2001
From: "Levytskyy, Vyacheslav" <vyacheslav.levytskyy at intel.com>
Date: Mon, 2 Sep 2024 02:00:24 -0700
Subject: [PATCH 1/4] branching: improve correctness of emitted MIR between
passes
---
llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp | 6 ------
llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp | 16 ++++++++++++++++
llvm/lib/Target/SPIRV/SPIRVInstrInfo.td | 6 ++++--
llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp | 15 +++++++++++++--
4 files changed, 33 insertions(+), 10 deletions(-)
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..6bf08be6bb9194 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) {
>From 62a9f2113e44655a710620aa6cb021056cd6f62e Mon Sep 17 00:00:00 2001
From: "Levytskyy, Vyacheslav" <vyacheslav.levytskyy at intel.com>
Date: Mon, 2 Sep 2024 02:59:35 -0700
Subject: [PATCH 2/4] add tests
---
llvm/test/CodeGen/SPIRV/branching/OpSwitchBranches.ll | 6 +++++-
llvm/test/CodeGen/SPIRV/branching/OpSwitchEmpty.ll | 6 +++++-
llvm/test/CodeGen/SPIRV/branching/OpSwitchUnreachable.ll | 5 ++++-
.../CodeGen/SPIRV/branching/Two_OpSwitch_same_register.ll | 5 ++++-
4 files changed, 18 insertions(+), 4 deletions(-)
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 [
>From b4df7434fefb7ef4fa081c5d17f00466359f034c Mon Sep 17 00:00:00 2001
From: "Levytskyy, Vyacheslav" <vyacheslav.levytskyy at intel.com>
Date: Mon, 2 Sep 2024 03:03:11 -0700
Subject: [PATCH 3/4] add test
---
llvm/test/CodeGen/SPIRV/transcoding/GlobalFunAnnotate.ll | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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"
>From 93f447d2449bc421b80932cd0a609ab62b28f9b5 Mon Sep 17 00:00:00 2001
From: "Levytskyy, Vyacheslav" <vyacheslav.levytskyy at intel.com>
Date: Mon, 2 Sep 2024 05:15:18 -0700
Subject: [PATCH 4/4] clang-format
---
llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
index 6bf08be6bb9194..df1b75bc1cb9eb 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
@@ -839,7 +839,7 @@ static void processSwitches(MachineFunction &MF, SPIRVGlobalRegistry *GR,
// 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)
+ for (MachineBasicBlock *Succ : ClearAddressTaken)
Succ->setAddressTakenIRBlock(nullptr);
}
More information about the llvm-commits
mailing list