[SPIR-V] Fix generation of gMIR vs. SPIR-V code from utility methods #128159
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) ChangesThe SPIR-V Backend uses the same set of utility functions, mostly though not entirely from SPIRVGlobalRegistry, to generate gMIR and SPIR-V opcodes, depending on the current stage of translation. This is controlled by an explicit EmitIR flag rather than the current translation pass, and there are legacy pieces of code where the EmitIR flag is declared so that it has a default true value, allowing using utility functions without explicitly declaring their intent to work either in gMIR or in SPIR-V part of the lowering process. While it may be ok to leave this default EmitIR flag as is in generation of scalar integer/float types, as we don't expect to see any dependent opcodes derived from such OpTypeXXX instructions, using of EmitIR by default in aggregation types is a source of hidden logical flaws and actual issues. This PR provides a partial fix to the problem by removing default status of EmitIR, requiring a user call site to explicitly announce its intent to generate gMIR or SPIR-V code, fixes several cases of misuse of EmitIR, and, the most important, fixes a nasty logical error that breaks passing of actually asked EmitIR value by the default value in the Full diff: https://github.com/llvm/llvm-project/pull/128159.diff 6 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp b/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
index 7b897f7e34c6f..c65166902550c 100644
--- a/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVBuiltins.cpp
@@ -476,8 +476,9 @@ static bool buildSelectInst(MachineIRBuilder &MIRBuilder,
if (ReturnType->getOpcode() == SPIRV::OpTypeVector) {
unsigned Bits = GR->getScalarOrVectorBitWidth(ReturnType);
uint64_t AllOnes = APInt::getAllOnes(Bits).getZExtValue();
- TrueConst = GR->getOrCreateConsIntVector(AllOnes, MIRBuilder, ReturnType);
- FalseConst = GR->getOrCreateConsIntVector(0, MIRBuilder, ReturnType);
+ TrueConst =
+ GR->getOrCreateConsIntVector(AllOnes, MIRBuilder, ReturnType, true);
+ FalseConst = GR->getOrCreateConsIntVector(0, MIRBuilder, ReturnType, true);
} else {
TrueConst = GR->buildConstantInt(1, MIRBuilder, ReturnType);
FalseConst = GR->buildConstantInt(0, MIRBuilder, ReturnType);
@@ -1457,7 +1458,7 @@ static bool genWorkgroupQuery(const SPIRV::IncomingCall *Call,
ToTruncate = DefaultReg;
}
auto NewRegister =
- GR->buildConstantInt(DefaultValue, MIRBuilder, PointerSizeType);
+ GR->buildConstantInt(DefaultValue, MIRBuilder, PointerSizeType, true);
MIRBuilder.buildCopy(DefaultReg, NewRegister);
} else { // If it could be in range, we need to load from the given builtin.
auto Vec3Ty =
@@ -1492,13 +1493,14 @@ static bool genWorkgroupQuery(const SPIRV::IncomingCall *Call,
GR->assignSPIRVTypeToVReg(BoolType, CompareRegister, MIRBuilder.getMF());
// Use G_ICMP to check if idxVReg < 3.
- MIRBuilder.buildICmp(CmpInst::ICMP_ULT, CompareRegister, IndexRegister,
- GR->buildConstantInt(3, MIRBuilder, IndexType));
+ MIRBuilder.buildICmp(
+ CmpInst::ICMP_ULT, CompareRegister, IndexRegister,
+ GR->buildConstantInt(3, MIRBuilder, IndexType, true));
// Get constant for the default value (0 or 1 depending on which
// function).
Register DefaultRegister =
- GR->buildConstantInt(DefaultValue, MIRBuilder, PointerSizeType);
+ GR->buildConstantInt(DefaultValue, MIRBuilder, PointerSizeType, true);
// Get a register for the selection result (possibly a new temporary one).
Register SelectionResult = Call->ReturnRegister;
@@ -2277,7 +2279,7 @@ static bool buildNDRange(const SPIRV::IncomingCall *Call,
Const = GR->getOrCreateConstIntArray(0, Size, *MIRBuilder.getInsertPt(),
SpvFieldTy, *ST.getInstrInfo());
} else {
- Const = GR->buildConstantInt(0, MIRBuilder, SpvTy);
+ Const = GR->buildConstantInt(0, MIRBuilder, SpvTy, true);
}
if (!LocalWorkSize.isValid())
LocalWorkSize = Const;
diff --git a/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp b/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
index 78f6b188c45c1..e47dfddd55975 100644
--- a/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVCallLowering.cpp
@@ -669,7 +669,8 @@ bool SPIRVCallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
// Make sure there's a valid return reg, even for functions returning void.
if (!ResVReg.isValid())
ResVReg = MIRBuilder.getMRI()->createVirtualRegister(&SPIRV::iIDRegClass);
- SPIRVType *RetType = GR->assignTypeToVReg(OrigRetTy, ResVReg, MIRBuilder);
+ SPIRVType *RetType = GR->assignTypeToVReg(
+ OrigRetTy, ResVReg, MIRBuilder, SPIRV::AccessQualifier::ReadWrite, true);
// Emit the call instruction and its args.
auto MIB = MIRBuilder.buildInstr(CallOp)
diff --git a/llvm/lib/Target/SPIRV/SPIRVEmitNonSemanticDI.cpp b/llvm/lib/Target/SPIRV/SPIRVEmitNonSemanticDI.cpp
index b98cef0a4f07f..ee98af5cffe4c 100644
--- a/llvm/lib/Target/SPIRV/SPIRVEmitNonSemanticDI.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVEmitNonSemanticDI.cpp
@@ -193,7 +193,8 @@ bool SPIRVEmitNonSemanticDI::emitGlobalDI(MachineFunction &MF) {
};
const SPIRVType *VoidTy =
- GR->getOrCreateSPIRVType(Type::getVoidTy(*Context), MIRBuilder);
+ GR->getOrCreateSPIRVType(Type::getVoidTy(*Context), MIRBuilder,
+ SPIRV::AccessQualifier::ReadWrite, false);
const auto EmitDIInstruction =
[&](SPIRV::NonSemanticExtInst::NonSemanticExtInst Inst,
@@ -217,7 +218,8 @@ bool SPIRVEmitNonSemanticDI::emitGlobalDI(MachineFunction &MF) {
};
const SPIRVType *I32Ty =
- GR->getOrCreateSPIRVType(Type::getInt32Ty(*Context), MIRBuilder);
+ GR->getOrCreateSPIRVType(Type::getInt32Ty(*Context), MIRBuilder,
+ SPIRV::AccessQualifier::ReadWrite, false);
const Register DwarfVersionReg =
GR->buildConstantInt(DwarfVersion, MIRBuilder, I32Ty, false);
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
index e2f1b211caa5c..a09474a21534e 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.cpp
@@ -244,7 +244,8 @@ SPIRVGlobalRegistry::getOrCreateConstIntReg(uint64_t Val, SPIRVType *SpvType,
CurMF->getRegInfo().createGenericVirtualRegister(LLT::scalar(BitWidth));
CurMF->getRegInfo().setRegClass(Res, &SPIRV::iIDRegClass);
if (MIRBuilder)
- assignTypeToVReg(LLVMIntTy, Res, *MIRBuilder);
+ assignTypeToVReg(LLVMIntTy, Res, *MIRBuilder,
+ SPIRV::AccessQualifier::ReadWrite, true);
else
assignIntTypeToVReg(BitWidth, Res, *I, *TII);
DT.add(CI, CurMF, Res);
@@ -271,7 +272,8 @@ SPIRVGlobalRegistry::getOrCreateConstFloatReg(APFloat Val, SPIRVType *SpvType,
CurMF->getRegInfo().createGenericVirtualRegister(LLT::scalar(BitWidth));
CurMF->getRegInfo().setRegClass(Res, &SPIRV::fIDRegClass);
if (MIRBuilder)
- assignTypeToVReg(LLVMFloatTy, Res, *MIRBuilder);
+ assignTypeToVReg(LLVMFloatTy, Res, *MIRBuilder,
+ SPIRV::AccessQualifier::ReadWrite, true);
else
assignFloatTypeToVReg(BitWidth, Res, *I, *TII);
DT.add(CI, CurMF, Res);
@@ -878,12 +880,13 @@ SPIRVType *SPIRVGlobalRegistry::getOpTypeOpaque(const StructType *Ty,
});
}
-SPIRVType *SPIRVGlobalRegistry::getOpTypeStruct(const StructType *Ty,
- MachineIRBuilder &MIRBuilder,
- bool EmitIR) {
+SPIRVType *SPIRVGlobalRegistry::getOpTypeStruct(
+ const StructType *Ty, MachineIRBuilder &MIRBuilder,
+ SPIRV::AccessQualifier::AccessQualifier AccQual, bool EmitIR) {
SmallVector<Register, 4> FieldTypes;
for (const auto &Elem : Ty->elements()) {
- SPIRVType *ElemTy = findSPIRVType(toTypedPointer(Elem), MIRBuilder);
+ SPIRVType *ElemTy =
+ findSPIRVType(toTypedPointer(Elem), MIRBuilder, AccQual, EmitIR);
assert(ElemTy && ElemTy->getOpcode() != SPIRV::OpTypeVoid &&
"Invalid struct element type");
FieldTypes.push_back(getSPIRVTypeID(ElemTy));
@@ -1017,26 +1020,27 @@ SPIRVType *SPIRVGlobalRegistry::createSPIRVType(
if (Ty->isVoidTy())
return getOpTypeVoid(MIRBuilder);
if (Ty->isVectorTy()) {
- SPIRVType *El =
- findSPIRVType(cast<FixedVectorType>(Ty)->getElementType(), MIRBuilder);
+ SPIRVType *El = findSPIRVType(cast<FixedVectorType>(Ty)->getElementType(),
+ MIRBuilder, AccQual, EmitIR);
return getOpTypeVector(cast<FixedVectorType>(Ty)->getNumElements(), El,
MIRBuilder);
}
if (Ty->isArrayTy()) {
- SPIRVType *El = findSPIRVType(Ty->getArrayElementType(), MIRBuilder);
+ SPIRVType *El =
+ findSPIRVType(Ty->getArrayElementType(), MIRBuilder, AccQual, EmitIR);
return getOpTypeArray(Ty->getArrayNumElements(), El, MIRBuilder, EmitIR);
}
if (auto SType = dyn_cast<StructType>(Ty)) {
if (SType->isOpaque())
return getOpTypeOpaque(SType, MIRBuilder);
- return getOpTypeStruct(SType, MIRBuilder, EmitIR);
+ return getOpTypeStruct(SType, MIRBuilder, AccQual, EmitIR);
}
if (auto FType = dyn_cast<FunctionType>(Ty)) {
- SPIRVType *RetTy = findSPIRVType(FType->getReturnType(), MIRBuilder);
+ SPIRVType *RetTy =
+ findSPIRVType(FType->getReturnType(), MIRBuilder, AccQual, EmitIR);
SmallVector<SPIRVType *, 4> ParamTypes;
- for (const auto &t : FType->params()) {
- ParamTypes.push_back(findSPIRVType(t, MIRBuilder));
- }
+ for (const auto &ParamTy : FType->params())
+ ParamTypes.push_back(findSPIRVType(ParamTy, MIRBuilder, AccQual, EmitIR));
return getOpTypeFunction(RetTy, ParamTypes, MIRBuilder);
}
diff --git a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
index 0c94ec4df97f5..5ad705f197d5a 100644
--- a/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
+++ b/llvm/lib/Target/SPIRV/SPIRVGlobalRegistry.h
@@ -94,13 +94,11 @@ class SPIRVGlobalRegistry {
// Add a new OpTypeXXX instruction without checking for duplicates.
SPIRVType *createSPIRVType(const Type *Type, MachineIRBuilder &MIRBuilder,
- SPIRV::AccessQualifier::AccessQualifier AQ =
- SPIRV::AccessQualifier::ReadWrite,
- bool EmitIR = true);
+ SPIRV::AccessQualifier::AccessQualifier AQ,
+ bool EmitIR);
SPIRVType *findSPIRVType(const Type *Ty, MachineIRBuilder &MIRBuilder,
- SPIRV::AccessQualifier::AccessQualifier accessQual =
- SPIRV::AccessQualifier::ReadWrite,
- bool EmitIR = true);
+ SPIRV::AccessQualifier::AccessQualifier accessQual,
+ bool EmitIR);
SPIRVType *
restOfCreateSPIRVType(const Type *Type, MachineIRBuilder &MIRBuilder,
SPIRV::AccessQualifier::AccessQualifier AccessQual,
@@ -321,9 +319,8 @@ class SPIRVGlobalRegistry {
// and map it to the given VReg by creating an ASSIGN_TYPE instruction.
SPIRVType *assignTypeToVReg(const Type *Type, Register VReg,
MachineIRBuilder &MIRBuilder,
- SPIRV::AccessQualifier::AccessQualifier AQ =
- SPIRV::AccessQualifier::ReadWrite,
- bool EmitIR = true);
+ SPIRV::AccessQualifier::AccessQualifier AQ,
+ bool EmitIR);
SPIRVType *assignIntTypeToVReg(unsigned BitWidth, Register VReg,
MachineInstr &I, const SPIRVInstrInfo &TII);
SPIRVType *assignFloatTypeToVReg(unsigned BitWidth, Register VReg,
@@ -470,13 +467,14 @@ class SPIRVGlobalRegistry {
MachineIRBuilder &MIRBuilder);
SPIRVType *getOpTypeArray(uint32_t NumElems, SPIRVType *ElemType,
- MachineIRBuilder &MIRBuilder, bool EmitIR = true);
+ MachineIRBuilder &MIRBuilder, bool EmitIR);
SPIRVType *getOpTypeOpaque(const StructType *Ty,
MachineIRBuilder &MIRBuilder);
SPIRVType *getOpTypeStruct(const StructType *Ty, MachineIRBuilder &MIRBuilder,
- bool EmitIR = true);
+ SPIRV::AccessQualifier::AccessQualifier AccQual,
+ bool EmitIR);
SPIRVType *getOpTypePointer(SPIRV::StorageClass::StorageClass SC,
SPIRVType *ElemType, MachineIRBuilder &MIRBuilder,
@@ -539,7 +537,7 @@ class SPIRVGlobalRegistry {
SPIRVType *SpvType,
const SPIRVInstrInfo &TII);
Register getOrCreateConsIntVector(uint64_t Val, MachineIRBuilder &MIRBuilder,
- SPIRVType *SpvType, bool EmitIR = true);
+ SPIRVType *SpvType, bool EmitIR);
Register getOrCreateConstNullPtr(MachineIRBuilder &MIRBuilder,
SPIRVType *SpvType);
Register buildConstantSampler(Register Res, unsigned AddrMode, unsigned Param,
diff --git a/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp b/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
index d5b81bf46c804..f622be893919f 100644
--- a/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVISelLowering.cpp
@@ -192,7 +192,7 @@ static void validateGroupWaitEventsPtr(const SPIRVSubtarget &STI,
// Insert a bitcast before the instruction to keep SPIR-V code valid.
LLVMContext &Context = MF->getFunction().getContext();
SPIRVType *NewPtrType =
- createNewPtrType(GR, I, OpType, false, true, nullptr,
+ createNewPtrType(GR, I, OpType, false, false, nullptr,
TargetExtType::get(Context, "spirv.Event"));
doInsertBitcast(STI, MRI, GR, I, OpReg, OpIdx, NewPtrType);
}
@@ -216,7 +216,8 @@ static void validateLifetimeStart(const SPIRVSubtarget &STI,
MachineIRBuilder MIB(I);
LLVMContext &Context = MF->getFunction().getContext();
SPIRVType *ElemType =
- GR.getOrCreateSPIRVType(IntegerType::getInt8Ty(Context), MIB);
+ GR.getOrCreateSPIRVType(IntegerType::getInt8Ty(Context), MIB,
+ SPIRV::AccessQualifier::ReadWrite, false);
SPIRVType *NewPtrType = GR.getOrCreateSPIRVPointerType(ElemType, MIB, SC);
doInsertBitcast(STI, MRI, GR, I, PtrReg, 0, NewPtrType);
}
|
MrSidims
approved these changes
Feb 25, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/16387 Here is the relevant piece of the build log for the reference |
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.
The SPIR-V Backend uses the same set of utility functions, mostly though not entirely from SPIRVGlobalRegistry, to generate gMIR and SPIR-V opcodes, depending on the current stage of translation. This is controlled by an explicit EmitIR flag rather than the current translation pass, and there are legacy pieces of code where the EmitIR flag is declared so that it has a default true value, allowing using utility functions without explicitly declaring their intent to work either in gMIR or in SPIR-V part of the lowering process.
While it may be ok to leave this default EmitIR flag as is in generation of scalar integer/float types, as we don't expect to see any dependent opcodes derived from such OpTypeXXX instructions, using of EmitIR by default in aggregation types is a source of hidden logical flaws and actual issues.
This PR provides a partial fix to the problem by removing default status of EmitIR, requiring a user call site to explicitly announce its intent to generate gMIR or SPIR-V code, fixes several cases of misuse of EmitIR, and, the most important, fixes a nasty logical error that breaks passing of actually asked EmitIR value by the default value in the middle of the chain of calls, in the
findSPIRVTypecall. The latter error was a source of issues in the post-instruction selection pass that has been getting gMIR code where SPIR-V was explicitly requested due to overloaded with default parameters internal API in SPIRVGlobalRegistry (most notably,findSPIRVType).