[SPIR-V] Implement correct zeroinitializer for extension types in SPIR-V Backend #93607
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 implements correct zeroinitializer for extension types in SPIR-V Backend. Previous version has just created 0 of 32/64 integer type (depending on target machine word size), that caused re-use and type re-write of the corresponding integer constant 0 with a potential crash on wrong usage of the constant (i.e., 0 of integer type expected but extension type found). E.g., the following code would crash without the PR: because 0 in icmp would eventually be of Full diff: https://github.com/llvm/llvm-project/pull/93607.diff 3 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
index ea53fe55e7ab5..46ef287b741c0 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitIntrinsics.cpp
@@ -1191,8 +1191,7 @@ void SPIRVEmitIntrinsics::processInstrAfterVisit(Instruction *I,
B.SetInsertPoint(I);
Value *OpTyVal = Op;
if (Op->getType()->isTargetExtTy())
- OpTyVal = Constant::getNullValue(
- IntegerType::get(I->getContext(), GR->getPointerSize()));
+ OpTyVal = UndefValue::get(Op->getType());
auto *NewOp = buildIntrWithMD(Intrinsic::spv_track_constant,
{Op->getType(), OpTyVal->getType()}, Op,
OpTyVal, {}, B);
diff --git a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
index 85299a49a6b94..624899600693a 100644
--- a/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVPreLegalizer.cpp
@@ -40,6 +40,7 @@ class SPIRVPreLegalizer : public MachineFunctionPass {
static void
addConstantsToTrack(MachineFunction &MF, SPIRVGlobalRegistry *GR,
+ const SPIRVSubtarget &STI,
DenseMap<MachineInstr *, Type *> &TargetExtConstTypes) {
MachineRegisterInfo &MRI = MF.getRegInfo();
DenseMap<MachineInstr *, Register> RegsAlreadyAddedToDT;
@@ -82,8 +83,17 @@ addConstantsToTrack(MachineFunction &MF, SPIRVGlobalRegistry *GR,
if (Const->getType()->isTargetExtTy()) {
// remember association so that we can restore it when assign types
MachineInstr *SrcMI = MRI.getVRegDef(SrcReg);
- if (SrcMI && SrcMI->getOpcode() == TargetOpcode::G_CONSTANT)
+ if (SrcMI && (SrcMI->getOpcode() == TargetOpcode::G_CONSTANT ||
+ SrcMI->getOpcode() == TargetOpcode::G_IMPLICIT_DEF))
TargetExtConstTypes[SrcMI] = Const->getType();
+ if (Const->isNullValue()) {
+ MachineIRBuilder MIB(MF);
+ SPIRVType *ExtType =
+ GR->getOrCreateSPIRVType(Const->getType(), MIB);
+ SrcMI->setDesc(STI.getInstrInfo()->get(SPIRV::OpConstantNull));
+ SrcMI->addOperand(MachineOperand::CreateReg(
+ GR->getSPIRVTypeID(ExtType), false));
+ }
}
} else {
RegsAlreadyAddedToDT[&MI] = Reg;
@@ -394,6 +404,7 @@ generateAssignInstrs(MachineFunction &MF, SPIRVGlobalRegistry *GR,
for (auto MII = std::prev(MBB->end()), Begin = MBB->begin();
!ReachedBegin;) {
MachineInstr &MI = *MII;
+ unsigned MIOp = MI.getOpcode();
if (isSpvIntrinsic(MI, Intrinsic::spv_assign_ptr_type)) {
Register Reg = MI.getOperand(1).getReg();
@@ -419,9 +430,9 @@ generateAssignInstrs(MachineFunction &MF, SPIRVGlobalRegistry *GR,
if (Def->getOpcode() != TargetOpcode::G_GLOBAL_VALUE)
insertAssignInstr(Reg, Ty, nullptr, GR, MIB, MF.getRegInfo());
ToErase.push_back(&MI);
- } else if (MI.getOpcode() == TargetOpcode::G_CONSTANT ||
- MI.getOpcode() == TargetOpcode::G_FCONSTANT ||
- MI.getOpcode() == TargetOpcode::G_BUILD_VECTOR) {
+ } else if (MIOp == TargetOpcode::G_CONSTANT ||
+ MIOp == TargetOpcode::G_FCONSTANT ||
+ MIOp == TargetOpcode::G_BUILD_VECTOR) {
// %rc = G_CONSTANT ty Val
// ===>
// %cty = OpType* ty
@@ -435,15 +446,15 @@ generateAssignInstrs(MachineFunction &MF, SPIRVGlobalRegistry *GR,
continue;
}
Type *Ty = nullptr;
- if (MI.getOpcode() == TargetOpcode::G_CONSTANT) {
+ if (MIOp == TargetOpcode::G_CONSTANT) {
auto TargetExtIt = TargetExtConstTypes.find(&MI);
Ty = TargetExtIt == TargetExtConstTypes.end()
? MI.getOperand(1).getCImm()->getType()
: TargetExtIt->second;
- } else if (MI.getOpcode() == TargetOpcode::G_FCONSTANT) {
+ } else if (MIOp == TargetOpcode::G_FCONSTANT) {
Ty = MI.getOperand(1).getFPImm()->getType();
} else {
- assert(MI.getOpcode() == TargetOpcode::G_BUILD_VECTOR);
+ assert(MIOp == TargetOpcode::G_BUILD_VECTOR);
Type *ElemTy = nullptr;
MachineInstr *ElemMI = MRI.getVRegDef(MI.getOperand(1).getReg());
assert(ElemMI);
@@ -459,7 +470,7 @@ generateAssignInstrs(MachineFunction &MF, SPIRVGlobalRegistry *GR,
Ty = VectorType::get(ElemTy, NumElts, false);
}
insertAssignInstr(Reg, Ty, nullptr, GR, MIB, MRI);
- } else if (MI.getOpcode() == TargetOpcode::G_GLOBAL_VALUE) {
+ } else if (MIOp == TargetOpcode::G_GLOBAL_VALUE) {
propagateSPIRVType(&MI, GR, MRI, MIB);
}
@@ -802,7 +813,7 @@ bool SPIRVPreLegalizer::runOnMachineFunction(MachineFunction &MF) {
MachineIRBuilder MIB(MF);
// a registry of target extension constants
DenseMap<MachineInstr *, Type *> TargetExtConstTypes;
- addConstantsToTrack(MF, GR, TargetExtConstTypes);
+ addConstantsToTrack(MF, GR, ST, TargetExtConstTypes);
foldConstantsIntoIntrinsics(MF);
insertBitcasts(MF, GR, MIB);
generateAssignInstrs(MF, GR, MIB, TargetExtConstTypes);
diff --git a/llvm/test/CodeGen/SPIRV/event-zero-const.ll b/llvm/test/CodeGen/SPIRV/event-zero-const.ll
new file mode 100644
index 0000000000000..b40456d233f12
--- /dev/null
+++ b/llvm/test/CodeGen/SPIRV/event-zero-const.ll
@@ -0,0 +1,23 @@
+; RUN: llc -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 %}
+
+; RUN: llc -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: %[[#LongTy:]] = OpTypeInt 64 0
+; CHECK: %[[#EventTy:]] = OpTypeEvent
+; CHECK: %[[#LongNull:]] = OpConstantNull %[[#LongTy]]
+; CHECK: %[[#EventNull:]] = OpConstantNull %[[#EventTy]]
+; CHECK: OpFunction
+; CHECK: OpINotEqual %[[#]] %[[#]] %[[#LongNull]]
+; CHECK: OpGroupAsyncCopy %[[#EventTy]] %[[#]] %[[#]] %[[#]] %[[#]] %[[#]] %[[#EventNull]]
+
+
+define weak_odr dso_local spir_kernel void @foo(i64 %_arg_i, ptr addrspace(1) %_arg_ptr, ptr addrspace(3) %_arg_local) {
+entry:
+ %r1 = icmp ne i64 %_arg_i, 0
+ %e1 = tail call spir_func target("spirv.Event") @__spirv_GroupAsyncCopy(i32 2, ptr addrspace(3) %_arg_local, ptr addrspace(1) %_arg_ptr, i64 1, i64 1, target("spirv.Event") zeroinitializer)
+ ret void
+}
+
+declare dso_local spir_func target("spirv.Event") @__spirv_GroupAsyncCopy(i32, ptr addrspace(3), ptr addrspace(1), i64, i64, target("spirv.Event"))
|
| @@ -1191,8 +1191,7 @@ void SPIRVEmitIntrinsics::processInstrAfterVisit(Instruction *I, | |||
| B.SetInsertPoint(I); | |||
| Value *OpTyVal = Op; | |||
| if (Op->getType()->isTargetExtTy()) | |||
| OpTyVal = Constant::getNullValue( | |||
| IntegerType::get(I->getContext(), GR->getPointerSize())); | |||
| OpTyVal = UndefValue::get(Op->getType()); | |||
There was a problem hiding this comment.
Prefer PoisonValue::get() unless you need "undef" for some specific reason.
There was a problem hiding this comment.
Thank you for the suggestion, applied.
michalpaszkowski
approved these changes
May 29, 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 implements correct zeroinitializer for extension types in SPIR-V Backend.
Previous version has just created 0 of 32/64 integer type (depending on target machine word size), that caused re-use and type re-write of the corresponding integer constant 0 with a potential crash on wrong usage of the constant (i.e., 0 of integer type expected but extension type found). E.g., the following code would crash without the PR:
because 0 in icmp would eventually be of
Eventtype.