[llvm] [CodeGen] commuteInstruction should update implicit-def (PR #131361)
Sander de Smalen via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 2 08:49:13 PDT 2025
https://github.com/sdesmalen-arm updated https://github.com/llvm/llvm-project/pull/131361
>From 8d23eec3283b69d35c374947ddb2ebedc006235a Mon Sep 17 00:00:00 2001
From: Sander de Smalen <sander.desmalen at arm.com>
Date: Mon, 10 Mar 2025 17:35:08 +0000
Subject: [PATCH 1/3] Precommit test
---
.../X86/coalesce-commutative-implicit-def.mir | 37 +++++++++++++++++++
1 file changed, 37 insertions(+)
create mode 100644 llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
diff --git a/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir b/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
new file mode 100644
index 0000000000000..130ae3867813c
--- /dev/null
+++ b/llvm/test/CodeGen/X86/coalesce-commutative-implicit-def.mir
@@ -0,0 +1,37 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 5
+# RUN: llc -mtriple=x86_64 -run-pass=register-coalescer -o - %s | FileCheck %s
+
+# When the coalescer removes the COPY by commuting the operands of the AND, it should also update the `implicit-def` of the destination register.
+---
+name: implicit_def_dst
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; 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 [[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`)
+ %1.sub_32bit:gr64_with_sub_8bit = AND32rr %1.sub_32bit:gr64_with_sub_8bit, %0.sub_32bit:gr64_with_sub_8bit, implicit-def dead $eflags, implicit-def %1:gr64_with_sub_8bit
+ %0:gr64_with_sub_8bit = COPY %1:gr64_with_sub_8bit
+ RET 0, implicit %0
+...
+# In case the MIR for some reason contains more than one implicit-def of the destination reg, then all should be updated.
+---
+name: two_implicit_defs_dst
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; CHECK-LABEL: name: two_implicit_defs_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 [[MOV64rm1]], 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`)
+ %1.sub_32bit:gr64_with_sub_8bit = AND32rr %1.sub_32bit:gr64_with_sub_8bit, %0.sub_32bit:gr64_with_sub_8bit, implicit-def dead $eflags, implicit-def %1:gr64_with_sub_8bit, implicit-def %1:gr64_with_sub_8bit
+ %0:gr64_with_sub_8bit = COPY %1:gr64_with_sub_8bit
+ RET 0, implicit %0
+...
>From d7abb47319157dd45abfe931bd123e6ba6b318de Mon Sep 17 00:00:00 2001
From: Sander de Smalen <sander.desmalen at arm.com>
Date: Mon, 10 Mar 2025 11:17:18 +0000
Subject: [PATCH 2/3] [CodeGen] commuteInstruction should update implicit-def
When the RegisterCoalescer adds an implicit-def when coalescing
a SUBREG_TO_REG (#123632), this causes issues when removing other
COPY nodes by commuting the instruction because it doesn't take
the implicit-def into consideration. This PR fixes that.
---
llvm/lib/CodeGen/TargetInstrInfo.cpp | 21 +++++++++++++++++++
.../X86/coalesce-commutative-implicit-def.mir | 4 ++--
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index 7e0a1e2a8a06e..cb30af2c026b2 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -214,6 +214,7 @@ 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 &&
@@ -228,6 +229,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.
@@ -240,6 +259,8 @@ MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
if (HasDef) {
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 130ae3867813c..fe1235fe94f85 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 [[MOV64rm1]]
+ ; 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: 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`)
@@ -27,7 +27,7 @@ body: |
; CHECK-LABEL: name: two_implicit_defs_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 [[MOV64rm1]], implicit-def [[MOV64rm1]]
+ ; 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]], implicit-def [[MOV64rm]]
; 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`)
>From 808ebbda9a5d280683caf8377c895451f60fe401 Mon Sep 17 00:00:00 2001
From: Sander de Smalen <sander.desmalen at arm.com>
Date: Wed, 2 Apr 2025 16:22:01 +0100
Subject: [PATCH 3/3] Use MachineInstr::substituteRegister
---
llvm/lib/CodeGen/TargetInstrInfo.cpp | 30 ++++++++--------------------
1 file changed, 8 insertions(+), 22 deletions(-)
diff --git a/llvm/lib/CodeGen/TargetInstrInfo.cpp b/llvm/lib/CodeGen/TargetInstrInfo.cpp
index cb30af2c026b2..7bccfb9cdf6c8 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -214,7 +214,6 @@ 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 &&
@@ -229,24 +228,6 @@ 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.
@@ -257,10 +238,15 @@ MachineInstr *TargetInstrInfo::commuteInstructionImpl(MachineInstr &MI,
}
if (HasDef) {
- CommutedMI->getOperand(0).setReg(Reg0);
+ // 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).setSubReg(SubReg0);
- for (unsigned Idx : UpdateImplicitDefIdx)
- CommutedMI->getOperand(Idx).setReg(Reg0);
}
CommutedMI->getOperand(Idx2).setReg(Reg1);
CommutedMI->getOperand(Idx1).setReg(Reg2);
More information about the llvm-commits
mailing list