-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[CodeGen] MachineVerifier to check early-clobber constraint #151421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs tests in test/MachineVerifier
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a property of the instruction definition and not require manual setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, still needs tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Currently MachineVerifier is missing verifying early-clobber operand constraint. The only other machine operand constraint - TiedTo is already verified.
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.