[SPIRV] Audit select Result in SPIRVInstructionSelector
#115193
Merged
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
- as per the definition of `select` in GlobalISel/InstructionSelector.h the return value is a boolean denoting if the select was successful - doing `Result |=` is incorrect as all inserted instructions should be succesful, hence we change to using `Result &=`
|
@llvm/pr-subscribers-backend-spir-v Author: Finn Plummer (inbelic) Changes
Full diff: https://github.com/llvm/llvm-project/pull/115193.diff 1 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
index be38b22f70c583..f5b9ca939ba898 100644
--- a/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVInstructionSelector.cpp
@@ -1082,16 +1082,16 @@ bool SPIRVInstructionSelector::selectAtomicRMW(Register ResVReg,
uint32_t MemSem = static_cast<uint32_t>(getMemSemantics(AO));
Register MemSemReg = buildI32Constant(MemSem /*| ScSem*/, I);
- bool Result = false;
+ bool Result = true;
Register ValueReg = I.getOperand(2).getReg();
if (NegateOpcode != 0) {
// Translation with negative value operand is requested
Register TmpReg = MRI->createVirtualRegister(&SPIRV::iIDRegClass);
- Result |= selectUnOpWithSrc(TmpReg, ResType, I, ValueReg, NegateOpcode);
+ Result &= selectUnOpWithSrc(TmpReg, ResType, I, ValueReg, NegateOpcode);
ValueReg = TmpReg;
}
- Result |= BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(NewOpcode))
+ Result &= BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(NewOpcode))
.addDef(ResVReg)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(Ptr)
@@ -1264,21 +1264,21 @@ bool SPIRVInstructionSelector::selectAtomicCmpXchg(Register ResVReg,
.constrainAllUses(TII, TRI, RBI);
Register CmpSuccReg = MRI->createVirtualRegister(&SPIRV::iIDRegClass);
SPIRVType *BoolTy = GR.getOrCreateSPIRVBoolType(I, TII);
- Result |= BuildMI(*I.getParent(), I, DL, TII.get(SPIRV::OpIEqual))
+ Result &= BuildMI(*I.getParent(), I, DL, TII.get(SPIRV::OpIEqual))
.addDef(CmpSuccReg)
.addUse(GR.getSPIRVTypeID(BoolTy))
.addUse(ACmpRes)
.addUse(Cmp)
.constrainAllUses(TII, TRI, RBI);
Register TmpReg = MRI->createVirtualRegister(&SPIRV::iIDRegClass);
- Result |= BuildMI(*I.getParent(), I, DL, TII.get(SPIRV::OpCompositeInsert))
+ Result &= BuildMI(*I.getParent(), I, DL, TII.get(SPIRV::OpCompositeInsert))
.addDef(TmpReg)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(ACmpRes)
.addUse(GR.getOrCreateUndef(I, ResType, TII))
.addImm(0)
.constrainAllUses(TII, TRI, RBI);
- Result |= BuildMI(*I.getParent(), I, DL, TII.get(SPIRV::OpCompositeInsert))
+ Result &= BuildMI(*I.getParent(), I, DL, TII.get(SPIRV::OpCompositeInsert))
.addDef(ResVReg)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(CmpSuccReg)
@@ -1784,7 +1784,7 @@ bool SPIRVInstructionSelector::selectDot4AddPackedExpansion(
assert(I.getOperand(4).isReg());
MachineBasicBlock &BB = *I.getParent();
- bool Result = false;
+ bool Result = true;
// Acc = C
Register Acc = I.getOperand(4).getReg();
@@ -1796,7 +1796,7 @@ bool SPIRVInstructionSelector::selectDot4AddPackedExpansion(
for (unsigned i = 0; i < 4; i++) {
// A[i]
Register AElt = MRI->createVirtualRegister(&SPIRV::IDRegClass);
- Result |= BuildMI(BB, I, I.getDebugLoc(), TII.get(ExtractOp))
+ Result &= BuildMI(BB, I, I.getDebugLoc(), TII.get(ExtractOp))
.addDef(AElt)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(I.getOperand(2).getReg())
@@ -1806,7 +1806,7 @@ bool SPIRVInstructionSelector::selectDot4AddPackedExpansion(
// B[i]
Register BElt = MRI->createVirtualRegister(&SPIRV::IDRegClass);
- Result |= BuildMI(BB, I, I.getDebugLoc(), TII.get(ExtractOp))
+ Result &= BuildMI(BB, I, I.getDebugLoc(), TII.get(ExtractOp))
.addDef(BElt)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(I.getOperand(3).getReg())
@@ -1816,7 +1816,7 @@ bool SPIRVInstructionSelector::selectDot4AddPackedExpansion(
// A[i] * B[i]
Register Mul = MRI->createVirtualRegister(&SPIRV::IDRegClass);
- Result |= BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpIMulS))
+ Result &= BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpIMulS))
.addDef(Mul)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(AElt)
@@ -1825,7 +1825,7 @@ bool SPIRVInstructionSelector::selectDot4AddPackedExpansion(
// Discard 24 highest-bits so that stored i32 register is i8 equivalent
Register MaskMul = MRI->createVirtualRegister(&SPIRV::IDRegClass);
- Result |= BuildMI(BB, I, I.getDebugLoc(), TII.get(ExtractOp))
+ Result &= BuildMI(BB, I, I.getDebugLoc(), TII.get(ExtractOp))
.addDef(MaskMul)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(Mul)
@@ -1836,7 +1836,7 @@ bool SPIRVInstructionSelector::selectDot4AddPackedExpansion(
// Acc = Acc + A[i] * B[i]
Register Sum =
i < 3 ? MRI->createVirtualRegister(&SPIRV::IDRegClass) : ResVReg;
- Result |= BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpIAddS))
+ Result &= BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpIAddS))
.addDef(Sum)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(Acc)
@@ -1907,7 +1907,7 @@ bool SPIRVInstructionSelector::selectSign(Register ResVReg,
if (NeedsConversion) {
auto ConvertOpcode = IsFloatTy ? SPIRV::OpConvertFToS : SPIRV::OpSConvert;
- Result |= BuildMI(*I.getParent(), I, DL, TII.get(ConvertOpcode))
+ Result &= BuildMI(*I.getParent(), I, DL, TII.get(ConvertOpcode))
.addDef(ResVReg)
.addUse(GR.getSPIRVTypeID(ResType))
.addUse(SignReg)
|
pow2clk
reviewed
Nov 6, 2024
There was a problem hiding this comment.
These changes all look good. There are a few instances of BuildMI being called without the result being read or otherwise used that you might consider while you're at it. I don't know why they would fail, but I don't know specifically why any of these would fail, but if they do, it should be passed up.
- propogate all BuildMI returns up propogate the buildmi result in buildI32Constant
|
Merged to get the splat test failure fix |
spall
reviewed
Nov 7, 2024
| } | ||
| // Build boolean value from the higher part. | ||
| Status &= BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpINotEqual)) | ||
| Result &= BuildMI(BB, I, I.getDebugLoc(), TII.get(SPIRV::OpINotEqual)) |
There was a problem hiding this comment.
I would do 'return Result && BuildMI', instead of returning later.
- make result return consistent to use return Result && when applicable
pow2clk
approved these changes
Nov 11, 2024
| else { | ||
| auto MemSemNeqConstant = buildI32Constant(MemSemEq, I); | ||
| MemSemNeqReg = MemSemNeqConstant.first; | ||
| Result &= MemSemNeqConstant.second; |
There was a problem hiding this comment.
I'm not going to insist on it, but just FYI, you could have made buildI32Constant return Register() in the failing case, which could be identified by checking Register.isValid() rather than returning an additional bool. It might have made this a little more elegant, but I'm not sure about that and it's entirely a non-functional change, so I'll happily defer to the implementer here.
There was a problem hiding this comment.
I agree it would be more elegant.
But from scanning the file, there doesn't seem to be a consistent precedent to checking a register's validity before use. In particular the returns of similar functions buildZerosVal and BuildOnesVal. I think to differentiate from those functions, and to ensure the result is checked in future calls we should explicitly return the type.
This might be a good reason to also audit the use of returned Registers, which I think is out of scope for this pr. Then we could switch to returning an invalid Register.
So I will merge as-is.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/126/builds/1073 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.
selectin GlobalISel/InstructionSelector.h the return value is a boolean denoting if the select was successfulResult |=is incorrect as all inserted instructions should be succesful, hence we change to usingResult &=