[CodeGen] Do not use subsituteRegister to update implicit def #148068
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
Member
Author
|
gentle ping @sdesmalen-arm and @arsenm |
Contributor
|
Needs test but could also be argued is just a substituteRegister bug |
Member
Author
Let me try to dig the pattern that cause the issue...... |
Member
Author
|
I think the issue is that The wrong fixes leads to (This happens since Since the original implementation only tries to update IIUC, the later one matches the behaviour of the previous implementation (i.e, without the |
Member
|
@llvm/pr-subscribers-backend-x86 Author: Peiming Liu (PeimingLiu) ChangesIt seems This PR simply reverts the original PR (#131361) to its initial implementation (without using Not sure whether it is a desired fix (and by no means that I am an expert on LLVM backend), but it does fix a numeric error on our internal workload. Original author: @sdesmalen-arm Full diff: https://github.com/llvm/llvm-project/pull/148068.diff 2 Files Affected:
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 660a1a4d7ec47..ce4234bf8a007 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -228,6 +228,24 @@ MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
SubReg0 = SubReg1;
}
+ // For a case like this:
+ // %0.sub = INST %0.sub(tied), %1.sub, implicit-def %0
+ // we need to update the implicit-def after commuting to result in:
+ // %1.sub = INST %1.sub(tied), %0.sub, implicit-def %1
+ SmallVector<unsigned> UpdateImplicitDefIdx;
+ if (HasDef && MI.hasImplicitDef() && MI.getOperand(0).getReg() != Reg0) {
+ const TargetRegisterInfo *TRI =
+ MI.getMF()->getSubtarget().getRegisterInfo();
+ Register OrigReg0 = MI.getOperand(0).getReg();
+ for (auto [OpNo, MO] : llvm::enumerate(MI.implicit_operands())) {
+ Register ImplReg = MO.getReg();
+ if ((ImplReg.isVirtual() && ImplReg == OrigReg0) ||
+ (ImplReg.isPhysical() && OrigReg0.isPhysical() &&
+ TRI->isSubRegisterEq(ImplReg, OrigReg0)))
+ UpdateImplicitDefIdx.push_back(OpNo + MI.getNumExplicitOperands());
+ }
+ }
+
MachineInstr *CommutedMI = nullptr;
if (NewMI) {
// Create a new instruction.
@@ -238,15 +256,10 @@ MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
}
if (HasDef) {
- // Use `substituteRegister` so that for a case like this:
- // %0.sub = INST %0.sub(tied), %1.sub, implicit-def %0
- // the implicit-def is also updated, to result in:
- // %1.sub = INST %1.sub(tied), %0.sub, implicit-def %1
- const TargetRegisterInfo &TRI =
- *MI.getMF()->getSubtarget().getRegisterInfo();
- Register FromReg = CommutedMI->getOperand(0).getReg();
- CommutedMI->substituteRegister(FromReg, Reg0, /*SubRegIdx=*/0, TRI);
+ CommutedMI->getOperand(0).setReg(Reg0);
CommutedMI->getOperand(0).setSubReg(SubReg0);
+ for (unsigned Idx : UpdateImplicitDefIdx)
+ CommutedMI->getOperand(Idx).setReg(Reg0);
}
CommutedMI->getOperand(Idx2).setReg(Reg1);
CommutedMI->getOperand(Idx1).setReg(Reg2);
diff --git a/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir b/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
index fe1235fe94f85..8de2beb6ba75a 100644
--- a/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
+++ b/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
@@ -10,7 +10,7 @@ body: |
; CHECK-LABEL: name: implicit_def_dst
; CHECK: [[MOV64rm:%[0-9]+]]:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64) from `ptr null`)
; CHECK-NEXT: [[MOV64rm1:%[0-9]+]]:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64) from `ptr null`)
- ; CHECK-NEXT: [[MOV64rm:%[0-9]+]].sub_32bit:gr64_with_sub_8bit = AND32rr [[MOV64rm]].sub_32bit, [[MOV64rm1]].sub_32bit, implicit-def dead $eflags, implicit-def [[MOV64rm]]
+ ; CHECK-NEXT: [[MOV64rm:%[0-9]+]].sub_32bit:gr64_with_sub_8bit = AND32rr [[MOV64rm]].sub_32bit, [[MOV64rm1]].sub_32bit, implicit-def dead $eflags, implicit-def [[MOV64rm1]]
; CHECK-NEXT: RET 0, implicit [[MOV64rm]]
%0:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64) from `ptr null`)
%1:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64) from `ptr null`)
@@ -35,3 +35,20 @@ body: |
%0:gr64_with_sub_8bit = COPY %1:gr64_with_sub_8bit
RET 0, implicit %0
...
+# Commuting instruction with 3 ops is handled correctly.
+---
+name: commuting_3_ops
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; CHECK-LABEL: name: commuting_3_ops
+ ; CHECK: [[DEF:%[0-9]+]]:vr256 = IMPLICIT_DEF
+ ; CHECK-NEXT: [[DEF1:%[0-9]+]]:vr256 = IMPLICIT_DEF
+ ; CHECK-NEXT: [[DEF:%[0-9]+]]:vr256 = contract nofpexcept VFMADD231PSYr [[DEF]], [[DEF]], [[DEF1]], implicit $mxcsr
+ ; CHECK-NEXT: RET 0, implicit [[DEF]]
+ %0:vr256 = IMPLICIT_DEF
+ %1:vr256 = IMPLICIT_DEF
+ %0:vr256 = contract nofpexcept VFMADD231PSYr %0:vr256(tied-def 0), %0:vr256, %1:vr256, implicit $mxcsr
+ %1:vr256 = COPY %0:vr256
+ RET 0, implicit %0
+...
|
PeimingLiu
force-pushed
the
users/peimingliu/partial-revert
branch
3 times, most recently
from
18a3e70 to
c038190
Compare
PeimingLiu
force-pushed
the
users/peimingliu/partial-revert
branch
from
c038190 to
7a347d8
Compare
Collaborator
sdesmalen-arm
left a comment
sdesmalen-arm
left a comment
There was a problem hiding this comment.
Thanks for raising this @PeimingLiu!
PeimingLiu
force-pushed
the
users/peimingliu/partial-revert
branch
from
b6740c7 to
5950e82
Compare
sdesmalen-arm
approved these changes
Jul 11, 2025
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/15471 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.
It seems
subsituteRegisterchecksFromReg == ToReginstead ofTRI->isSubRegisterEq.This PR simply reverts the original PR (#131361) to its initial implementation (without using
subsituteRegister).Not sure whether it is a desired fix (and by no means that I am an expert on LLVM backend), but it does fix a numeric error on our internal workload.
Original author: @sdesmalen-arm