[llvm] [CodeGen] commuteInstruction should update implicit-def (PR #131361)
Sander de Smalen via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 14 10:25:24 PDT 2025
https://github.com/sdesmalen-arm created https://github.com/llvm/llvm-project/pull/131361
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.
>From 100df3a537c0728c297aa2a7abb4c0866db7d5ce 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/2] 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 c98ea57f63e0976ec07a28a38b3a2ec34a165178 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/2] [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 7a905b65f26e5..5a1cd5afa7ef6 100644
--- a/llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ b/llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -200,6 +200,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 &&
@@ -214,6 +215,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.
@@ -226,6 +245,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`)
More information about the llvm-commits
mailing list