From db3674ad0e538e106788331297e3b96d1766cd08 Mon Sep 17 00:00:00 2001 From: Peiming Liu 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 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 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 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 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 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 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())) {