[llvm] [CodeGen] Do not use subsituteRegister to update implicit def (PR #148068)
Peiming Liu via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 11 09:53:30 PDT 2025
https://github.com/PeimingLiu updated https://github.com/llvm/llvm-project/pull/148068
>From db3674ad0e538e106788331297e3b96d1766cd08 Mon Sep 17 00:00:00 2001
From: Peiming Liu <peiming at modular.com>
Date: Thu, 10 Jul 2025 15:02:07 -0700
Subject: [PATCH 1/4] [CodeGen] Do not use subsituteRegister to update implicit
def
---
llvm/lib/CodeGen/TargetInstrInfo.cpp | 29 ++++++++++++++++++++--------
1 file changed, 21 insertions(+), 8 deletions(-)
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);
>From 7a347d8410ed566e87f169ec363325fbca99f817 Mon Sep 17 00:00:00 2001
From: Peiming Liu <peiming at modular.com>
Date: Thu, 10 Jul 2025 19:10:36 -0700
Subject: [PATCH 2/4] add test
---
.../X86/coalesce-commutative-implicit-def.mir | 21 +++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir b/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
index fe1235fe94f85..1f38430f631cc 100644
--- a/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
+++ b/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
@@ -35,3 +35,24 @@ 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:
+ liveins: $ymm0, $ymm1
+
+ ; CHECK-LABEL: name: commuting_3_ops
+ ; CHECK: liveins: $ymm0, $ymm1
+ ; CHECK-NEXT: {{ $}}
+ ; CHECK-NEXT: [[COPY:%[0-9]+]]:vr256 = COPY $ymm1
+ ; CHECK-NEXT: [[COPY1:%[0-9]+]]:vr256 = COPY $ymm0
+ ; CHECK-NEXT: [[COPY1:%[0-9]+]]:vr256 = contract nofpexcept VFMADD213PSYr [[COPY1]], [[COPY]], [[COPY]], implicit $mxcsr
+ ; CHECK-NEXT: RET 0, implicit [[COPY1]]
+ %0:vr256 = COPY $ymm1
+ %1:vr256 = COPY $ymm0
+ %0:vr256 = contract nofpexcept VFMADD231PSYr %0:vr256, %0:vr256, %1:vr256, implicit $mxcsr
+ %1:vr256 = COPY %0:vr256
+ RET 0, implicit %1
+...
>From 5950e8298adeb6014a918713e27df1634c53aeb4 Mon Sep 17 00:00:00 2001
From: Peiming Liu <peiming at modular.com>
Date: Fri, 11 Jul 2025 08:59:37 -0700
Subject: [PATCH 3/4] address comment
---
llvm/lib/CodeGen/TargetInstrInfo.cpp | 36 ++++++++++++++--------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index ce4234bf8a007..e7224b6f6a054 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -214,38 +214,38 @@ MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
Reg1.isPhysical() ? MI.getOperand(Idx1).isRenamable() : false;
bool Reg2IsRenamable =
Reg2.isPhysical() ? MI.getOperand(Idx2).isRenamable() : false;
- // If destination is tied to either of the commuted source register, then
- // it must be updated.
- if (HasDef && Reg0 == Reg1 &&
- MI.getDesc().getOperandConstraint(Idx1, MCOI::TIED_TO) == 0) {
- Reg2IsKill = false;
- Reg0 = Reg2;
- SubReg0 = SubReg2;
- } else if (HasDef && Reg0 == Reg2 &&
- MI.getDesc().getOperandConstraint(Idx2, MCOI::TIED_TO) == 0) {
- Reg1IsKill = false;
- Reg0 = Reg1;
- 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) {
+ 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)))
+ if ((ImplReg.isVirtual() && ImplReg == Reg0) ||
+ (ImplReg.isPhysical() && Reg0.isPhysical() &&
+ TRI->isSubRegisterEq(ImplReg, Reg0)))
UpdateImplicitDefIdx.push_back(OpNo + MI.getNumExplicitOperands());
}
}
+ // If destination is tied to either of the commuted source register, then
+ // it must be updated.
+ if (HasDef && Reg0 == Reg1 &&
+ MI.getDesc().getOperandConstraint(Idx1, MCOI::TIED_TO) == 0) {
+ Reg2IsKill = false;
+ Reg0 = Reg2;
+ SubReg0 = SubReg2;
+ } else if (HasDef && Reg0 == Reg2 &&
+ MI.getDesc().getOperandConstraint(Idx2, MCOI::TIED_TO) == 0) {
+ Reg1IsKill = false;
+ Reg0 = Reg1;
+ SubReg0 = SubReg1;
+ }
+
MachineInstr *CommutedMI = nullptr;
if (NewMI) {
// Create a new instruction.
>From df33a7987b7982ff56f9e1c9b94fa98b0193cf3d Mon Sep 17 00:00:00 2001
From: Peiming Liu <peiming at modular.com>
Date: Fri, 11 Jul 2025 09:53:16 -0700
Subject: [PATCH 4/4] remove useless check
---
llvm/lib/CodeGen/TargetInstrInfo.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index e7224b6f6a054..518a9339d8d11 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -220,7 +220,7 @@ MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
// 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) {
+ if (HasDef && MI.hasImplicitDef()) {
const TargetRegisterInfo *TRI =
MI.getMF()->getSubtarget().getRegisterInfo();
for (auto [OpNo, MO] : llvm::enumerate(MI.implicit_operands())) {
More information about the llvm-commits
mailing list