[MCP] Do not remove redundant copy for COPY from undef

[Pierre-vh]

Summary

I don't think we can safely remove the second COPY as redundant in such cases.
The first COPY (which has undef src) may be lowered to a KILL instruction instead, resulting in no COPY being emitted at all.

Testcase is X86 so it's in the same place as other testcases for this function, but this was initially spotted on AMDGPU with the following:

renamable $vgpr24 = PRED_COPY undef renamable $vgpr25, implicit $exec
renamable $vgpr24 = PRED_COPY killed renamable $vgpr25, implicit $exec

The second COPY waas removed as redundant, and the first one was lowered to a KILL (= removed too), causing $vgpr24 to not have $vgpr25's value.

Fixes SWDEV-401507

[arsenm]

llvm/test/CodeGen/X86/machine-copy-prop.mir

232

I don’t see why this test isn’t failing the verifier. There’s no def of rdi, so the second and third uses should also be undef.

For the one read is undef and later are not, there still needs to be a live in of some kind

[arsenm]

llvm/test/CodeGen/X86/machine-copy-prop.mir

227

I think tracksRegLiveness strikes again (why is this an option?)

[Pierre-vh]

llvm/test/CodeGen/X86/machine-copy-prop.mir

227

Indeed, it's because tracksRegLiveness is false. If I set it to true it indeed fails the verifier.
I just copied and slightly modified a test in the same file, so I think it's alright. Would you prefer if I changed it? I guess I just need an implicit def of $rdi

[foad]

Code change LGTM.

[arsenm]

llvm/test/CodeGen/X86/machine-copy-prop.mir

227

Then all the tests are broken. They should all enable liveness

[arsenm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

466

Instead of not doing the fold, can you just strip the undef from the first copy?

[Pierre-vh]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

466

I think it should be the same in the end, but`DestSourcePair` only returns const operands, so I can't modify the undef flag from the previous COPY unless I change the members of that class to be non-const, or I directly access MI.getOperand(1) but then it defeats the purpose of having an abstraction over COPY instrs operands, IMO

I also think in the end it doesn't really matter, the undef COPY just gets deleted anyway. This is such a niche case already (no tests covered it, and it's only been discovered now randomly) that I would say let's not overthink it, maybe let's just add a TODO for it?

[Pierre-vh]

Fix liveness in test

llvm/lib/CodeGen/MachineCopyPropagation.cpp

bool MachineCopyPropagation::eraseIfRedundant(MachineInstr &Copy,
   // Search for an existing copy.
   MachineInstr *PrevCopy =
       Tracker.findAvailCopy(Copy, Def, *TRI, *TII, UseCopyInstr);
   if (!PrevCopy)
     return false;
 
   auto PrevCopyOperands = isCopyInstr(*PrevCopy, *TII, UseCopyInstr);
   // Check that the existing copy uses the correct sub registers.
-  if (PrevCopyOperands->Destination->isDead())
+  if (PrevCopyOperands->Destination->isDead() ||
+      PrevCopyOperands->Source->isUndef())
     return false;
   if (!isNopCopy(*PrevCopy, Src, Def, TRI, TII, UseCopyInstr))
     return false;
 
   LLVM_DEBUG(dbgs() << "MCP: copy is a NOP, removing: "; Copy.dump());
 
   // Copy was redundantly redefining either Src or Def. Remove earlier kill
   // flags between Copy and PrevCopy because the value will be reused now.

llvm/test/CodeGen/X86/machine-copy-prop.mir

 # RUN: llc -mtriple=i686-- -run-pass machine-cp -verify-machineinstrs -o - %s | FileCheck %s
 
 --- |
   declare void @foo()
   define void @copyprop_remove_kill0() { ret void }
   define void @copyprop_remove_kill1() { ret void }
   define void @copyprop_remove_kill2() { ret void }
   define void @copyprop0() { ret void }
   define void @copyprop1() { ret void }
   define void @copyprop2() { ret void }
   define void @nocopyprop0() { ret void }
   define void @nocopyprop1() { ret void }
   define void @nocopyprop2() { ret void }
   define void @nocopyprop3() { ret void }
   define void @nocopyprop4() { ret void }
   define void @nocopyprop5() { ret void }
+  define void @nocopyprop6() { ret void }
 ...
 ---
 # The second copy is redundant and will be removed, check that we also remove
 # the kill flag of intermediate instructions.
 # CHECK-LABEL: name: copyprop_remove_kill0
 # CHECK: bb.0:
 # CHECK-NEXT: $rax = COPY $rdi
 # CHECK-NEXT: NOOP implicit $rdi

 # CHECK-NEXT: $rip = COPY $rax
 # CHECK-NEXT: $rip = COPY $rax
 name: nocopyprop5
 body: |
   bb.0:
     $rip = COPY $rax
     $rip = COPY $rax
 ...
+---
+# The copies are obviously redundant, but the earlier COPY copies from "undef".
+# That copy may be lowered to a KILL instead and thus we cannot remove the
+# second COPY, otherwise we risk making $rax undef.
+# CHECK-LABEL: name: nocopyprop6
+# CHECK: bb.0:
+# CHECK:      $rax = COPY undef $rdi
+# CHECK-NEXT: NOOP implicit killed $rax
+# CHECK-NEXT: $rax = COPY $rdi
+# CHECK-NEXT: NOOP implicit $rax, implicit $rdi
+name: nocopyprop6
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $rdi
+
+    $rax = COPY undef $rdi
+    NOOP implicit killed $rax
+    $rax = COPY $rdi
+    NOOP implicit $rax, implicit $rdi
+...