Skip to content

Commit 9c29217

Browse files
[SPIR-V] Ensure that cleaning of temporary constants doesn't purge tracked constants (#95303)
This PR fixes a problem in logics of cleaning unused constants, ensuring that cleaning of temporary constants doesn't purge tracked constants. On a rare occasion when this happens SPIR-V Backend emits a code that refers to a non-existent register, earlier related with a constant. Attached to the PR test case is a minimal reproducer where names of variables and instructions lead to such a rare coincidence.
1 parent 4baea8b commit 9c29217

File tree

2 files changed

+37
-5
lines changed

2 files changed

+37
-5
lines changed

llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ class SPIRVPreLegalizer : public MachineFunctionPass {
4141
static void
4242
addConstantsToTrack(MachineFunction &MF, SPIRVGlobalRegistry *GR,
4343
const SPIRVSubtarget &STI,
44-
DenseMap<MachineInstr *, Type *> &TargetExtConstTypes) {
44+
DenseMap<MachineInstr *, Type *> &TargetExtConstTypes,
45+
SmallSet<Register, 4> &TrackedConstRegs) {
4546
MachineRegisterInfo &MRI = MF.getRegInfo();
4647
DenseMap<MachineInstr *, Register> RegsAlreadyAddedToDT;
4748
SmallVector<MachineInstr *, 10> ToErase, ToEraseComposites;
@@ -80,6 +81,7 @@ addConstantsToTrack(MachineFunction &MF, SPIRVGlobalRegistry *GR,
8081
}
8182
}
8283
GR->add(Const, &MF, SrcReg);
84+
TrackedConstRegs.insert(SrcReg);
8385
if (Const->getType()->isTargetExtTy()) {
8486
// remember association so that we can restore it when assign types
8587
MachineInstr *SrcMI = MRI.getVRegDef(SrcReg);
@@ -121,7 +123,9 @@ addConstantsToTrack(MachineFunction &MF, SPIRVGlobalRegistry *GR,
121123
MI->eraseFromParent();
122124
}
123125

124-
static void foldConstantsIntoIntrinsics(MachineFunction &MF) {
126+
static void
127+
foldConstantsIntoIntrinsics(MachineFunction &MF,
128+
const SmallSet<Register, 4> &TrackedConstRegs) {
125129
SmallVector<MachineInstr *, 10> ToErase;
126130
MachineRegisterInfo &MRI = MF.getRegInfo();
127131
const unsigned AssignNameOperandShift = 2;
@@ -137,7 +141,8 @@ static void foldConstantsIntoIntrinsics(MachineFunction &MF) {
137141
MI.removeOperand(NumOp);
138142
MI.addOperand(MachineOperand::CreateImm(
139143
ConstMI->getOperand(1).getCImm()->getZExtValue()));
140-
if (MRI.use_empty(ConstMI->getOperand(0).getReg()))
144+
Register DefReg = ConstMI->getOperand(0).getReg();
145+
if (MRI.use_empty(DefReg) && !TrackedConstRegs.contains(DefReg))
141146
ToErase.push_back(ConstMI);
142147
}
143148
}
@@ -836,8 +841,10 @@ bool SPIRVPreLegalizer::runOnMachineFunction(MachineFunction &MF) {
836841
MachineIRBuilder MIB(MF);
837842
// a registry of target extension constants
838843
DenseMap<MachineInstr *, Type *> TargetExtConstTypes;
839-
addConstantsToTrack(MF, GR, ST, TargetExtConstTypes);
840-
foldConstantsIntoIntrinsics(MF);
844+
// to keep record of tracked constants
845+
SmallSet<Register, 4> TrackedConstRegs;
846+
addConstantsToTrack(MF, GR, ST, TargetExtConstTypes, TrackedConstRegs);
847+
foldConstantsIntoIntrinsics(MF, TrackedConstRegs);
841848
insertBitcasts(MF, GR, MIB);
842849
generateAssignInstrs(MF, GR, MIB, TargetExtConstTypes);
843850
processSwitches(MF, GR, MIB);
Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
; This test case ensures that cleaning of temporary constants doesn't purge tracked ones.
2+
3+
; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s --check-prefix=CHECK-SPIRV
4+
; RUN: %if spirv-tools %{ llc -O0 -mtriple=spirv64-unknown-unknown %s -o - -filetype=obj | spirv-val %}
5+
6+
; CHECK-SPIRV: %[[#Int:]] = OpTypeInt 32 0
7+
; CHECK-SPIRV: %[[#C0:]] = OpConstant %[[#Int]] 0
8+
; CHECK-SPIRV: %[[#C1:]] = OpConstant %[[#Int]] 1
9+
; CHECK-SPIRV: OpSelect %[[#Int]] %[[#]] %[[#C1]] %[[#C0]]
10+
11+
12+
define spir_kernel void @foo() {
13+
entry:
14+
%addr = alloca i32
15+
%r1 = call i8 @_Z20__spirv_SpecConstantia(i32 0, i8 1)
16+
; The name '%conv17.i' is important for the test case,
17+
; because it includes i32 0 when encoded for SPIR-V usage.
18+
%conv17.i = sext i8 %r1 to i64
19+
%tobool = trunc i8 %r1 to i1
20+
%r2 = zext i1 %tobool to i32
21+
store i32 %r2, ptr %addr
22+
ret void
23+
}
24+
25+
declare i8 @_Z20__spirv_SpecConstantia(i32, i8)

0 commit comments

Comments
 (0)