[llvm] [RegisterCoalescer] Don't commute two-address instructions which only define a subregister (PR #169031)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-x86

Author: None (KRM7)

<details>
<summary>Changes</summary>

Currently, the register coalescer may try to commute an instruction like:
```
%0.sub_lo32:gpr64 = AND %0.sub_lo32:gpr64(tied-def 0), %1.sub_lo32:gpr64
USE %0:gpr64
```
resulting in:
```
%1.sub_lo32:gpr64 = AND %1.sub_lo32:gpr64(tied-def 0), %0.sub_lo32:gpr64
USE %1:gpr64
```
However, this is not correct if the instruction doesn't define the entire register, as the value of the upper 32-bits
of the register used in `USE` will not be the same.

---
Full diff: https://github.com/llvm/llvm-project/pull/169031.diff


3 Files Affected:

- (modified) llvm/lib/CodeGen/RegisterCoalescer.cpp (+8) 
- (modified) llvm/test/CodeGen/SystemZ/regcoal-undef-lane-4-rm-cp-commuting-def.mir (+2-1) 
- (added) llvm/test/CodeGen/X86/coalesce-commutative-tied-def-subreg.mir (+43) 


``````````diff
diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index 25c4375a73ce0..4560afa07d114 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -869,6 +869,14 @@ RegisterCoalescer::removeCopyByCommutingDef(const CoalescerPair &CP,
   if (!DefMI->isRegTiedToUseOperand(DefIdx, &UseOpIdx))
     return {false, false};
 
+  // If DefMI only defines the register partially, we can't replace uses of the
+  // full register with the new destination register after commuting it.
+  if (IntA.reg().isVirtual() &&
+      none_of(DefMI->all_defs(), [&](const MachineOperand &DefMO) {
+        return DefMO.getReg() == IntA.reg() && !DefMO.getSubReg();
+      }))
+    return {false, false};
+
   // FIXME: The code below tries to commute 'UseOpIdx' operand with some other
   // commutable operand which is expressed by 'CommuteAnyOperandIndex'value
   // passed to the method. That _other_ operand is chosen by
diff --git a/llvm/test/CodeGen/SystemZ/regcoal-undef-lane-4-rm-cp-commuting-def.mir b/llvm/test/CodeGen/SystemZ/regcoal-undef-lane-4-rm-cp-commuting-def.mir
index cd8f14b28844b..8eb119c4ca662 100644
--- a/llvm/test/CodeGen/SystemZ/regcoal-undef-lane-4-rm-cp-commuting-def.mir
+++ b/llvm/test/CodeGen/SystemZ/regcoal-undef-lane-4-rm-cp-commuting-def.mir
@@ -59,7 +59,8 @@ body:             |
   ; CHECK-NEXT:   [[LAY2:%[0-9]+]]:gr64bit = OGR [[LAY2]], [[LAY]], implicit-def dead $cc
   ; CHECK-NEXT:   undef [[AHIMuxK:%[0-9]+]].subreg_l32:gr64bit = AHIMuxK [[LGHI]].subreg_l32, 19843, implicit-def dead $cc
   ; CHECK-NEXT:   [[AHIMuxK:%[0-9]+]]:gr64bit = ROSBG [[AHIMuxK]], [[LAY2]], 32, 63, 0, implicit-def dead $cc
-  ; CHECK-NEXT:   [[DEF:%[0-9]+]].subreg_l32:gr64bit = OR [[DEF]].subreg_l32, [[AHIMuxK]].subreg_l32, implicit-def dead $cc
+  ; CHECK-NEXT:   [[AHIMuxK:%[0-9]+]].subreg_l32:gr64bit = OR [[AHIMuxK]].subreg_l32, [[DEF]].subreg_l32, implicit-def dead $cc
+  ; CHECK-NEXT:   [[DEF:%[0-9]+]]:gr64bit = COPY [[AHIMuxK]]
   ; CHECK-NEXT:   [[DEF:%[0-9]+]]:gr64bit = ROSBG [[DEF]], [[LAY1]], 32, 63, 0, implicit-def dead $cc
   ; CHECK-NEXT:   CHIMux [[LHIMux1]], 0, implicit-def $cc
   ; CHECK-NEXT:   BRC 14, 6, %bb.6, implicit killed $cc
diff --git a/llvm/test/CodeGen/X86/coalesce-commutative-tied-def-subreg.mir b/llvm/test/CodeGen/X86/coalesce-commutative-tied-def-subreg.mir
new file mode 100644
index 0000000000000..59560f318ff3d
--- /dev/null
+++ b/llvm/test/CodeGen/X86/coalesce-commutative-tied-def-subreg.mir
@@ -0,0 +1,43 @@
+# 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
+
+# The COPY can't be removed by commuting the AND since the instruction only partially defines the register
+---
+name: commute_tied_subreg_def_part
+tracksRegLiveness: true
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: commute_tied_subreg_def_part
+    ; CHECK: %A:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (volatile load (s64))
+    ; CHECK-NEXT: %B:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64))
+    ; CHECK-NEXT: %A.sub_32bit:gr64_with_sub_8bit = AND32rr %A.sub_32bit, %B.sub_32bit, implicit-def dead $eflags
+    ; CHECK-NEXT: MOV64mr $noreg, 1, $noreg, 0, $noreg, %A :: (store (s64))
+    ; CHECK-NEXT: %B:gr64_with_sub_8bit = COPY %A
+    ; CHECK-NEXT: RET 0, implicit %B
+    %A:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (volatile load (s64))
+    %B:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64))
+    %A.sub_32bit:gr64_with_sub_8bit = AND32rr %A.sub_32bit, %B.sub_32bit, implicit-def dead $eflags
+    MOV64mr $noreg, 1, $noreg, 0, $noreg, %A :: (store (s64))
+    %B:gr64_with_sub_8bit = COPY %A
+    RET 0, implicit %B
+...
+
+# The COPY can be removed by commuting the AND since the instruction defines the entire register
+---
+name: commute_tied_subreg_def_full
+tracksRegLiveness: true
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: commute_tied_subreg_def_full
+    ; CHECK: %A:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (volatile load (s64))
+    ; CHECK-NEXT: %B:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64))
+    ; CHECK-NEXT: %B.sub_32bit:gr64_with_sub_8bit = AND32rr %B.sub_32bit, %A.sub_32bit, implicit-def dead $eflags, implicit-def %B
+    ; CHECK-NEXT: MOV64mr $noreg, 1, $noreg, 0, $noreg, %B :: (store (s64))
+    ; CHECK-NEXT: RET 0, implicit %B
+    %A:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (volatile load (s64))
+    %B:gr64_with_sub_8bit = MOV64rm $noreg, 1, $noreg, 0, $noreg :: (load (s64))
+    %A.sub_32bit:gr64_with_sub_8bit = AND32rr %A.sub_32bit, %B.sub_32bit, implicit-def dead $eflags, implicit-def %A
+    MOV64mr $noreg, 1, $noreg, 0, $noreg, %A :: (store (s64))
+    %B:gr64_with_sub_8bit = COPY %A
+    RET 0, implicit %B
+...

``````````

</details>


https://github.com/llvm/llvm-project/pull/169031