[AArch64] Fix incorrect removal of COPYs in AArch64RedundantCopyElimination

[yutsumi]

Summary

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

Removing the following mov instruction generated from a COPY
is invalid if the upper 32 bits of x1 is non-zero and x1 will be used.
So, we should avoid this removal.

cbnz w1, b2
b1:
mov x1, wzr // set all 64 bits of x1 zero
// code assuming upper half of x1 is non-zero

The same fix https://reviews.llvm.org/D29850 as this was abandoned
probably because the above instruction seemed not to be generated.
But the generation of it is depending on the passes executed
before, e.g., register allocator.
So, this assumption should not be made and we should
avoid removing the above COPYs as well as MOVi32imms.

[dmgreen]

Do you have a example of this going wrong from a ll file or C example? I can see how this case is incorrect, but I'd like to try and trace through a full example and understand how these things get created.

[yutsumi]

@dmgreen No. I don't have ll file or C/C++ code now.

[dmgreen]

What was the original case that was going wrong then?

[yutsumi]

After my colleague modified llvm/lib/CodeGen/CalcSpillWeights.cpp for the register allocator in the LLVM 7 based compiler, he faced the Segmentation fault when executing a program.
As a result of an investigation, we found that the modification for the register allocator is not bad, and this fix is needed.
The above modification for the register allocator cannot be applied to the current main branch straightly, and cannot be published because that modification is in our product code.

[yutsumi]

Ping.

[dmgreen]

OK. This does alter codegen without changed to the register allocator, and I wanted to look into why.

What would happen if we had this? We should still know that the top bits are 0:

  $w8 = ...
  CBNZW $w8, %bb.2
bb.1:
  $w8 = COPY $wzr, implicit-def $x8
  ...

[yutsumi]

Sorry for the late reply.
I found that the problem this patch intended to fix was fixed by https://reviews.llvm.org/D87771 .

llvm/lib/Target/AArch64/AArch64RedundantCopyElimination.cpp

if (IsCopy || IsMoveImm) {
 
           if (IsMoveImm) {
             // For a move immediate, the known immediate must match the source
             // immediate.
             if (KnownReg.Imm != SrcImm)
               continue;
 
             // Don't remove a move immediate that implicitly defines the upper
-            // bits when only the lower 32 bits are known.
-            MCPhysReg CmpReg = KnownReg.Reg;
-            if (any_of(MI->implicit_operands(), [CmpReg](MachineOperand &O) {
-                  return !O.isDead() && O.isReg() && O.isDef() &&
-                         O.getReg() != CmpReg;
-                }))
-              continue;
+            // bits as different.
+            if (TRI->isSuperRegister(DefReg, KnownReg.Reg) && KnownReg.Imm < 0)
+              continue;
+          }
+
+          // Don't remove a move immediate or a copy that implicitly defines the
+          // upper bits when only the lower 32 bits are known.
 
-            // Don't remove a move immediate that implicitly defines the upper
-            // bits as different.
-            if (TRI->isSuperRegister(DefReg, KnownReg.Reg) && KnownReg.Imm < 0)
-              continue;
-          }
-
           if (IsCopy)
             LLVM_DEBUG(dbgs() << "Remove redundant Copy : " << *MI);
           else
             LLVM_DEBUG(dbgs() << "Remove redundant Move : " << *MI);
 
           MI->eraseFromParent();
           Changed = true;
           LastChange = I;

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

body:             |
   bb.1:
     liveins: $x1
 
     $w0 = MOVi32imm 4096
     STRWui killed $w0, killed $x1, 0
 
   bb.2:
     RET_ReallyLR
+...
+# Check that we don't remove a COPY that is implicitly defining the upper
+# 32 bits of an X reg if we don't know the upper 32 bits are zero.
+# CHECK-LABEL: name: test24
+# CHECK: COPY $wzr
+name:            test24
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $x0, $x1, $w0
+
+    $x0 = COPY $x1
+    CBNZW $w0, %bb.2
+
+  bb.1:
+    $w0 = COPY $wzr, implicit-def $x0
+    B %bb.3
+
+  bb.2:
+    liveins: $x1
+
+    $x0 = LDRXui $x1, 0
+
+  bb.3:
+    liveins: $x0
+
+    RET_ReallyLR implicit $x0
+...
+# Check that we do remove a COPY that is implicitly defining the upper
+# 32 bits of an X reg if we know the upper 32 bits are zero.
+# CHECK-LABEL: name: test25
+# CHECK-NOT: COPY $wzr
+name:            test25
+tracksRegLiveness: true
+body:             |
+  bb.0:
+    liveins: $x0, $x1, $w0
+
+    $x0 = COPY $x1
+    CBNZX $x0, %bb.2
+
+  bb.1:
+    $w0 = COPY $wzr, implicit-def $x0
+    B %bb.3
+
+  bb.2:
+    liveins: $x1
+
+    $x0 = LDRXui $x1, 0
+
+  bb.3:
+    liveins: $x0
+
+    RET_ReallyLR implicit $x0