[AArch64] Fix bug in AArch64RedundantCopyElimination removing COPYs it shouldn't.

[gberry]

Summary

Don't remove COPYs with implicit defs, unless we know the implicitly
defined register is also zero.

[t.p.northover]

Where do copies like that come from? It seems like a pretty sketchy target-specific extension to a shared instruction, and we should probably be representing that in some better way.

[MatzeB]

In D29850#674079, @t.p.northover wrote:

Where do copies like that come from? It seems like a pretty sketchy target-specific extension to a shared instruction, and we should probably be representing that in some better way.

Indeed a COPY instruction does not necessarily clear the upper bits of a register, even if the targets move instruction to implement it does. This mostly comes from the fact that we are allowed to eliminate NOP copies when the source and target are the same, with the rule to clear the upper bits we would not longer be allowed to do so.

[junbuml]

test/CodeGen/AArch64/machine-copy-remove.mir

24

Why not adding the below case as well in the test ?

%w0 = COPY %wzr, implicit-def %x0

[gberry]

Yeah, it looks like this was a false alarm. I spot checked some of the cases that were leading to these COPYs and all of them were of the form:

%vreg:sub_32<def,read-undef> = COPY %WZR

which don't expect to have the upper 32 bits cleared.

lib/Target/AArch64/AArch64RedundantCopyElimination.cpp

bool AArch64RedundantCopyElimination::optimizeCopy(MachineBasicBlock *MBB) {
 
   bool Changed = false;
   MachineBasicBlock::iterator LastChange = MBB->begin();
   unsigned SmallestDef = TargetReg;
   // Remove redundant Copy instructions unless TargetReg is modified.
   for (MachineBasicBlock::iterator I = MBB->begin(), E = MBB->end(); I != E;) {
     MachineInstr *MI = &*I;
     ++I;
-    if (MI->isCopy() && MI->getOperand(0).isReg() &&
-        MI->getOperand(1).isReg()) {
+    if (MI->isCopy()) {
 
       unsigned DefReg = MI->getOperand(0).getReg();
       unsigned SrcReg = MI->getOperand(1).getReg();
 
       if ((SrcReg == AArch64::XZR || SrcReg == AArch64::WZR) &&
           !MRI->isReserved(DefReg) &&
-          (TargetReg == DefReg || TRI->isSuperRegister(DefReg, TargetReg))) {
+          (TargetReg == DefReg || TRI->isSuperRegister(DefReg, TargetReg)) &&
+          // Don't remove COPYs with implicit defs that aren't known zero.  This
+          // can happen if e.g. this is a W reg COPY and we're depending on it
+          // zeroing out the high 32-bits of the corresponding X reg but we
+          // don't know the full contents of the X reg because the CBZ is on the
+          // W subreg.
+          all_of(MI->implicit_operands(), [TargetReg](MachineOperand &O) {
+            return !O.isDef() || O.getReg() == TargetReg;
+          })) {
         DEBUG(dbgs() << "Remove redundant Copy : ");
         DEBUG((MI)->print(dbgs()));
 
         MI->eraseFromParent();
         Changed = true;
         LastChange = I;
         NumCopiesRemoved++;
         SmallestDef =

test/CodeGen/AArch64/machine-copy-remove.mir

+# RUN: llc -mtriple=aarch64--linux-gnu -run-pass=aarch64-copyelim %s -verify-machineinstrs -o - 2>&1 | FileCheck %s
+
+--- |
+  define void @f_WWimpdef() { ret void }
+  define void @f_WWimpdef2() { ret void }
+...
+---
+# Make sure we don't remove a COPY that is implicitly defining the top
+# 32-bits of an X reg if we don't know the top 32-bits are zero.
+# CHECK-LABEL: name: f_WWimpdef
+# CHECK: %w0 = COPY %xzr, implicit-def %x0
+name:            f_WWimpdef
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1, %bb.2
+    liveins: %w0, %x1
+
+    CBNZW %w0, %bb.2
+
+  bb.1:
+    successors: %bb.3
+
+    %w0 = COPY %xzr, implicit-def %x0
+    B %bb.3
+
+  bb.2:
+    successors: %bb.3
+    liveins: %x1
+
+    %x0 = LDRXui %x1, 0 :: (load 8)
+
+  bb.3:
+    liveins: %x0
+
+    RET_ReallyLR implicit %x0
+
+...
+# Make sure we do remove a COPY that is implicitly defining the top
+# 32-bits of an X reg if we know the top 32-bits are zero.
+# CHECK-LABEL: name: f_WWimpdef2
+# CHECK-NOT: %w0 = COPY %xzr, implicit-def %x0
+name:            f_WWimpdef2
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    successors: %bb.1, %bb.2
+    liveins: %x0, %x1
+
+    CBNZX %x0, %bb.2
+
+  bb.1:
+    successors: %bb.3
+
+    %w0 = COPY %xzr, implicit-def %x0
+    B %bb.3
+
+  bb.2:
+    successors: %bb.3
+    liveins: %x1
+
+    %x0 = LDRXui %x1, 0 :: (load 8)
+
+  bb.3:
+    liveins: %x0
+
+    RET_ReallyLR implicit %x0
+
+...