[GlobalISel] Fix equality for copies from physregs in matchEqualDefs

[paquette]

Summary

When we see this:

%a = COPY $physreg
...
SOMETHING implicit-def $physreg
...
%b = COPY $physreg

The two copies are not equivalent, and so we shouldn't perform any folding on them.

When we have two instructions which use a physical register check that they define the same virtual register(s) as well.

e.g., if we run into this case

%a = COPY $physreg
...
%b = COPY %a

we can say that the two copies are the same, and can be folded.

[arsenm]

No test with the implicit def case you mentioned?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

1528

ISel should probably not ever think about liveness like this

[paquette]

The existing test had

%a:_(s32) = COPY $w0
%b:_(s32) = COPY $w0

which is no longer folded

but yeah it would probably be better to be clear in the testcase about why this isn't folded, so I'll change it.

[paquette]

• Remove FIXME, since we don't want to care about register liveness in isel
• Update testcase so that there's an implicit def between the copies
• Add another testcase that has a copy between the copies, because that should probably be tested too

[arsenm]

LGTM

llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-select.mir

3–10

The IR section shouldn't be needed?

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

if (!MOP1.isReg() || !MOP2.isReg())
     return false;
   MachineInstr *I1 = getDefIgnoringCopies(MOP1.getReg(), MRI);
   if (!I1)
     return false;
   MachineInstr *I2 = getDefIgnoringCopies(MOP2.getReg(), MRI);
   if (!I2)
     return false;
 
+  // Check for physical registers on the instructions first to avoid cases like
+  // this:
+  //
+  // %a = COPY $physreg
+  // ...
+  // SOMETHING implicit-def $physreg
+  // ...
+  // %b = COPY $physreg
+  //
+  // These copies are not equivalent.
+  if (any_of(I1->uses(), [](const MachineOperand &MO) {
+        return MO.isReg() && MO.getReg().isPhysical();
+      })) {
+    // Check if we have a case like this:
+    //
+    // %a = COPY $physreg
+    // %b = COPY %a
+    //
+    // In this case, I1 and I2 will both be equal to %a = COPY $physreg.
+    // From that, we know that they must have the same value, since they must
+    // have come from the same COPY.
+    return I1->isIdenticalTo(*I2);
+  }
+
+  // We don't have any physical registers, so we don't necessarily need the
+  // same vreg defs.
+  //
   // On the off-chance that there's some target instruction feeding into the
-  // select, let's use produceSameValue instead of isIdenticalTo.
+  // instruction, let's use produceSameValue instead of isIdenticalTo.
   return Builder.getTII().produceSameValue(*I1, *I2, &MRI);
 }
 
 bool CombinerHelper::replaceSingleDefInstWithOperand(MachineInstr &MI,
                                                      unsigned OpIdx) {
   assert(MI.getNumExplicitDefs() == 1 && "Expected one explicit def?");
   Register OldReg = MI.getOperand(0).getReg();
   Register Replacement = MI.getOperand(OpIdx).getReg();

llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-select.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple aarch64 -run-pass=aarch64-prelegalizer-combiner -verify-machineinstrs %s -o - | FileCheck %s
-
+--- |
+  declare void @foo()
+  define void @self() { ret void }
+  define void @self_with_copy() { ret void }
+  define void @self_not_equivalent_overwrite_w0() { ret void }
+  define void @self_not_equivalent_overwrite_w0_implicit() { ret void }
+  define void @self_not_equivalent_different_copies() { ret void }
+...
+---
 name:            self
 tracksRegLiveness: true
 body:             |
-  bb.1.entry:
+  bb.0:
     liveins: $w0, $w1
     ; Optimize (cond ? %a : %a) -> %a
     ; CHECK-LABEL: name: self
     ; CHECK: liveins: $w0, $w1
     ; CHECK: %a:_(s32) = COPY $w0
     ; CHECK: $w0 = COPY %a(s32)
     ; CHECK: RET_ReallyLR implicit $w0
     %a:_(s32) = COPY $w0
     %cond_wide:gpr(s32) = COPY $w1
     %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
     %select:_(s32) = G_SELECT %cond(s1), %a, %a
     $w0 = COPY %select(s32)
     RET_ReallyLR implicit $w0
 
 ...
 ---
 name:            self_with_copy
 tracksRegLiveness: true
 body:             |
-  bb.1.entry:
+  bb.0:
     liveins: $w0, $w1
     ; Optimize (cond ? %a : %b) -> %a
     ;
     ; This shows that we are looking through copies correctly and deduce that
     ; %b is a copy from %a.
     ;
     ; CHECK-LABEL: name: self_with_copy
     ; CHECK: liveins: $w0, $w1
     ; CHECK: %a:_(s32) = COPY $w0
     ; CHECK: $w0 = COPY %a(s32)
     ; CHECK: RET_ReallyLR implicit $w0
     %a:_(s32) = COPY $w0
     %b:_(s32) = COPY %a
     %cond_wide:gpr(s32) = COPY $w1
     %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
     %select:_(s32) = G_SELECT %cond(s1), %a, %b
     $w0 = COPY %select(s32)
     RET_ReallyLR implicit $w0
 
 ...
 ---
-name:            self_with_equivalent
+name:            self_not_equivalent_overwrite_w0
 tracksRegLiveness: true
 body:             |
-  bb.1.entry:
+  bb.0:
     liveins: $w0, $w1
-    ; Optimize (cond ? %a : %b) -> %a
     ;
-    ; This shows that we can detect when %a == %b, even though they define
-    ; different virtual registers.
+    ; $w0 is overwritten by a copy from $w1, so the copies for %a and %b are
+    ; not the same.
     ;
-    ; CHECK-LABEL: name: self_with_equivalent
+    ; CHECK-LABEL: name: self_not_equivalent_overwrite_w0
     ; CHECK: liveins: $w0, $w1
     ; CHECK: %a:_(s32) = COPY $w0
-    ; CHECK: $w0 = COPY %a(s32)
+    ; CHECK: $w0 = COPY $w1
+    ; CHECK: %b:_(s32) = COPY $w0
+    ; CHECK: %cond_wide:gpr(s32) = COPY $w1
+    ; CHECK: %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
+    ; CHECK: %select:_(s32) = G_SELECT %cond(s1), %a, %b
+    ; CHECK: $w0 = COPY %select(s32)
     ; CHECK: RET_ReallyLR implicit $w0
     %a:_(s32) = COPY $w0
+    $w0 = COPY $w1
     %b:_(s32) = COPY $w0
     %cond_wide:gpr(s32) = COPY $w1
     %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
     %select:_(s32) = G_SELECT %cond(s1), %a, %b
     $w0 = COPY %select(s32)
     RET_ReallyLR implicit $w0
 
 ...
 ---
-name:            self_not_equivalent
+name:            self_not_equivalent_overwrite_w0_implicit
 tracksRegLiveness: true
 body:             |
-  bb.1.entry:
+  bb.0:
+    liveins: $w0, $w1
+    ;
+    ; $w0 is overwritten by a call which defines it implicitly, so the copies
+    ; are not the same.
+    ;
+    ; CHECK-LABEL: name: self_not_equivalent_overwrite_w0_implicit
+    ; CHECK: liveins: $w0, $w1
+    ; CHECK: %a:_(s32) = COPY $w0
+    ; CHECK: BL @foo, implicit-def $w0
+    ; CHECK: %b:_(s32) = COPY $w0
+    ; CHECK: %cond_wide:gpr(s32) = COPY $w1
+    ; CHECK: %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
+    ; CHECK: %select:_(s32) = G_SELECT %cond(s1), %a, %b
+    ; CHECK: $w0 = COPY %select(s32)
+    ; CHECK: RET_ReallyLR implicit $w0
+    %a:_(s32) = COPY $w0
+    BL @foo, implicit-def $w0
+    %b:_(s32) = COPY $w0
+    %cond_wide:gpr(s32) = COPY $w1
+    %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
+    %select:_(s32) = G_SELECT %cond(s1), %a, %b
+    $w0 = COPY %select(s32)
+    RET_ReallyLR implicit $w0
+
+...
+---
+name:            self_not_equivalent_different_copies
+tracksRegLiveness: true
+body:             |
+  bb.0:
     liveins: $w0, $w1
     ; In this case, the copies are not equivalent, so there is no optimization.
-    ; CHECK-LABEL: name: self_not_equivalent
+    ;
+    ; CHECK-LABEL: name: self_not_equivalent_different_copies
     ; CHECK: liveins: $w0, $w1
     ; CHECK: %a:_(s32) = COPY $w0
     ; CHECK: %b:_(s32) = COPY $w1
     ; CHECK: %cond_wide:gpr(s32) = COPY $w1
     ; CHECK: %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
     ; CHECK: %select:_(s32) = G_SELECT %cond(s1), %a, %b
     ; CHECK: $w0 = COPY %select(s32)
     ; CHECK: RET_ReallyLR implicit $w0
     %a:_(s32) = COPY $w0
     %b:_(s32) = COPY $w1
+    %c:_(s32) = COPY %b
     %cond_wide:gpr(s32) = COPY $w1
     %cond:gpr(s1) = G_TRUNC %cond_wide(s32)
-    %select:_(s32) = G_SELECT %cond(s1), %a, %b
+    %select:_(s32) = G_SELECT %cond(s1), %a, %c
     $w0 = COPY %select(s32)
     RET_ReallyLR implicit $w0
+
+...