[CodeGen] MachineVerifier to check early-clobber constraint #151421
Conversation
|
Test failures on linux and windows are the same test |
|
@ mbrkusanin I see you have a PR on AMDGPU global Isel. I tried locating the bug (failure with early-clobber not set when G_AMDGPU_MAD_U64_U32 is emitted) and I tried fixing in legalizer llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp line 4091. but that is not sufficient. I could not locate (with quick searches) where this is emitted in Global ISel. any help appreciated. |
AbhayKanhere
force-pushed
the
eng/abhay/machine-verify-early-clobber
branch
from
984f262 to
a0a0504
Compare
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-backend-amdgpu Author: Abhay Kanhere (AbhayKanhere) ChangesCurrently MachineVerifier is missing verifying early-clobber operand constraint. Full diff: https://github.com/llvm/llvm-project/pull/151421.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index 01703fe09b79a..ebef1c9034f4a 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -2325,6 +2325,13 @@ void MachineVerifier::visitMachineInstrBefore(const MachineInstr *MI) {
report("Missing mayStore flag", MI);
}
+ // Verify earlyClobber def operand
+ if (MCID.getOperandConstraint(0, MCOI::EARLY_CLOBBER) != -1) {
+ if (!MI->getOperand(0).isReg())
+ report("Early clobber must be a register", MI);
+ if (!MI->getOperand(0).isEarlyClobber())
+ report("Missing earlyClobber flag", MI);
+ }
// Debug values must not have a slot index.
// Other instructions must have one, unless they are inside a bundle.
if (LiveInts) {
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp b/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
index 1a63c48e3666c..8a6ef89a4a062 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp
@@ -584,6 +584,7 @@ bool AMDGPUInstructionSelector::selectG_AMDGPU_MAD_64_32(
I.setDesc(TII.get(Opc));
I.addOperand(*MF, MachineOperand::CreateImm(0));
I.addImplicitDefUseOperands(*MF);
+ I.getOperand(0).setIsEarlyClobber(true);
return constrainSelectedInstRegOperands(I, TII, TRI, RBI);
}
|
|
This exposed more test fails someone with AMDGPU knowledge should look into whether this is expected. ! 2025-08-04T18:08:20.5815070Z Failed Tests (10): |
|
FYI I fixed these 10 tests locally and found 11 more. .. Wanted some feedback if this is worth fixing or not. |
arsenm
left a comment
arsenm
left a comment
There was a problem hiding this comment.
Needs tests in test/MachineVerifier
There was a problem hiding this comment.
This should be a property of the instruction definition and not require manual setting
There was a problem hiding this comment.
earlyClobber and TiedTo are two of the OperandConstraint -- we check for tiedTo but not earlyClobber in machine verifier. on CPU targets register allocators strictly look at earlyClobber on operand. Recently we discovered that when MI.setDesc is used to modify the opcode, this constraint on Operand is not updated. However if new instruction is built and operand added using addOperand() , then earlyClobber is set correctly.. Looks like AMDGPU backend makes heavy use of setDesc (since the earlySelect expects in-place change to opcode) but does not update earlyClobber. I found many instances of this.
There was a problem hiding this comment.
Internally we discussed a variant for MI.setDesc(MCID, bool updateOpConstraint=false) that updates OperandConstraints if passed true, but setDesc is often part of a set of update operations on operands and not the last in the sequence so it is not always possible to update operand constraints there.
There was a problem hiding this comment.
The problem here is that MI.setDesc which only updates opcode, and MI.addOperand following MI.buildInstr/ that create new instruction behave differently in how Operand constraints are updated.
There was a problem hiding this comment.
@arsenm It appears there is no way to write machine verifier test for this early clobber property (except by adding a fake test pass that generates bad IR without earlyclobber constraint set on machine operand) since when MIR is loaded, this constraint gets set -- so there is no way to feed in a MIR with unset early-clobber. This verifier has proved its usefulness with errors it has flagged as seen in these uses of MI.setDesc here and in our internal downstream code. I'd like to get this approved to merge in.
There was a problem hiding this comment.
Can you use inline asm? It is also possible to write unit tests, but it's more annoying
There was a problem hiding this comment.
Inline asm will not work either - early clobber is set in IR. inline asm are passed on to assembler intact.
AbhayKanhere
force-pushed
the
eng/abhay/machine-verify-early-clobber
branch
from
a0a0504 to
f70f3ba
Compare
|
I tried machine verifier test to check early-clobber is present. but any test .mir where earlyclobber is not set on instruction when MIR is read-in, that attribute gets set correctly via addOperands. So I am not able to test missing early-clobber crash! |
Currently MachineVerifier is missing verifying early-clobber operand constraint. The only other machine operand constraint - TiedTo is already verified. Fix the test failure with this PR, update AMDGPU code that did not set earlyclobber
AbhayKanhere
force-pushed
the
eng/abhay/machine-verify-early-clobber
branch
from
5624107 to
20a897a
Compare
|
updated tests, modifications based on review comments. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/187/builds/13307 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/41946 Here is the relevant piece of the build log for the reference |
|
Based on fails. I think I need to add changes removed in SIFoldOperands on AMDGPU. But I cannot reproduce these locally? I will file another PR with suggested fix. |
After PR:llvm#151421 merged following fails in SIFoldOperands showed up. LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.ll LLVM :: CodeGen/AMDGPU/mfma-loop.ll LLVM :: CodeGen/AMDGPU/rewrite-vgpr-mfma-to-agpr.ll In Folding code, if folded operand is register ensure earlyClobber is set.
|
FYI #166600 to fix test failures seen |
|
the expensive check builder (https://lab.llvm.org/buildbot/#/builders/187) is still "red" for 2 days already because of PR. Please fix it ASAP or revert your changes. |
|
@vvereschaka Yes the fix PR is under review. Thanks. #166600 once merged will fix the tests failing. thanks! |
After PR:#151421 merged following fails in SIFoldOperands showed up. LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.ll LLVM :: CodeGen/AMDGPU/mfma-loop.ll LLVM :: CodeGen/AMDGPU/rewrite-vgpr-mfma-to-agpr.ll In Folding code, if folded operand is register ensure earlyClobber is set. --------- Co-authored-by: Matt Arsenault <[email protected]> Co-authored-by: Shilei Tian <[email protected]>
…(#166600) After PR:llvm/llvm-project#151421 merged following fails in SIFoldOperands showed up. LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.ll LLVM :: CodeGen/AMDGPU/mfma-loop.ll LLVM :: CodeGen/AMDGPU/rewrite-vgpr-mfma-to-agpr.ll In Folding code, if folded operand is register ensure earlyClobber is set. --------- Co-authored-by: Matt Arsenault <[email protected]> Co-authored-by: Shilei Tian <[email protected]>
After PR:llvm#151421 merged following fails in SIFoldOperands showed up. LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.gfx90a.ll LLVM :: CodeGen/AMDGPU/llvm.amdgcn.mfma.ll LLVM :: CodeGen/AMDGPU/mfma-loop.ll LLVM :: CodeGen/AMDGPU/rewrite-vgpr-mfma-to-agpr.ll In Folding code, if folded operand is register ensure earlyClobber is set. --------- Co-authored-by: Matt Arsenault <[email protected]> Co-authored-by: Shilei Tian <[email protected]>
5 participants
Currently MachineVerifier is missing verifying early-clobber operand constraint.
The only other machine operand constraint - TiedTo is already verified.