[llvm] 4b73838 - [CodeGen] Do not use subsituteRegister to update implicit def (#148068)

[via llvm-commits]

Author: Peiming Liu
Date: 2025-07-11T11:28:14-07:00
New Revision: 4b738387aeba477aa3c7c7d688fa9982cf1f2722

URL: https://github.com/llvm/llvm-project/commit/4b738387aeba477aa3c7c7d688fa9982cf1f2722
DIFF: https://github.com/llvm/llvm-project/commit/4b738387aeba477aa3c7c7d688fa9982cf1f2722.diff

LOG: [CodeGen] Do not use subsituteRegister to update implicit def (#148068)

It seems `subsituteRegister` checks `FromReg == ToReg` instead of
`TRI->isSubRegisterEq`.

This PR simply reverts the original PR
(https://github.com/llvm/llvm-project/pull/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

Added: 
    

Modified: 
    llvm/lib/CodeGen/TargetInstrInfo.cpp
    llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 660a1a4d7ec47..518a9339d8d11 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -214,6 +214,24 @@ MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
       Reg1.isPhysical() ? MI.getOperand(Idx1).isRenamable() : false;
   bool Reg2IsRenamable =
       Reg2.isPhysical() ? MI.getOperand(Idx2).isRenamable() : false;
+
+  // 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()) {
+    const TargetRegisterInfo *TRI =
+        MI.getMF()->getSubtarget().getRegisterInfo();
+    for (auto [OpNo, MO] : llvm::enumerate(MI.implicit_operands())) {
+      Register ImplReg = MO.getReg();
+      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 &&
@@ -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..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
+...