[MCP] Optimize copies from undef

[Pierre-vh]

Summary

Revert D152502 and instead optimize away copy from undefs, but clear the undef flag on the original copy.
Apparently, not optimizing the COPY can cause performance issues in some cases.

Fixes SWDEV-405813, SWDEV-405899

[foad]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

486–487

Doesn't PrevCopyOperands->Source->setIsUndef(false) work?

[Pierre-vh]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

486–487

The reference to the MachineOperand is const, unfortunately

[arsenm]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

486–487

You can change that?

[Pierre-vh]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

486–487

It's struct DestSourcePair, it's sometimes initialized from const MachineInstr so I can't just remove the const qualifier there.

An alternative may be to create a copy of that struct and name it MutableDestSourcePair, and use it where relevant.

[Pierre-vh]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

486–487

In particular, the issue is with isCopyInstr which is used to generate the value here. It takes a const instruction and many user also pass const instructions, so it'd be a bit of a refactor (not sure how big).

If we really want to avoid the code above, then I will need to create an overload of that function that takes a non-const instr and returns a MutableDestSourcePair.

IMO, the piece of code above is ok and not worth the refactor, but I'd also like to get this in soon as it's a blocker for several tickets so I have no strong opinions - I just need to know how to proceed :)

[foad]

llvm/lib/CodeGen/MachineCopyPropagation.cpp

486–487

I think the code above is fine as-is, now that you've explained the reason.

[Pierre-vh]

Update tests

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() ||
-      PrevCopyOperands->Source->isUndef())
+  if (PrevCopyOperands->Destination->isDead())
     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.
   std::optional<DestSourcePair> CopyOperands =
       isCopyInstr(Copy, *TII, UseCopyInstr);
   assert(CopyOperands);
 
   Register CopyDef = CopyOperands->Destination->getReg();
   assert(CopyDef == Src || CopyDef == Def);
   for (MachineInstr &MI :
        make_range(PrevCopy->getIterator(), Copy.getIterator()))
     MI.clearRegisterKills(CopyDef, TRI);
 
+  // Clear undef flag from remaining copy if needed.
+  if (!CopyOperands->Source->isUndef()) {
+    PrevCopy->getOperand(PrevCopyOperands->Source->getOperandNo())
+        .setIsUndef(false);
+  }
+
   Copy.eraseFromParent();
   Changed = true;
   ++NumDeletes;
   return true;
 }
 
 bool MachineCopyPropagation::isBackwardPropagatableRegClassCopy(
     const MachineInstr &Copy, const MachineInstr &UseI, unsigned UseIdx) {

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 @copyprop3() { ret void }
+  define void @copyprop4() { 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
body: |
   bb.0:
     $rax = COPY $rdi
     NOOP implicit killed $ax
     CALL64pcrel32 @foo, csr_64_rt_mostregs
     $rax = COPY $rdi
     NOOP implicit $rax, implicit $rdi
 ...
 ---
+# Check that undef is removed from the remaining copy because
+# the deleted COPY did not have it.
+# CHECK-LABEL: name: copyprop3
+# CHECK: bb.0:
+# CHECK:      $rax = COPY $rdi
+# CHECK-NEXT: NOOP implicit $rax
+# CHECK-NEXT: NOOP implicit $rax, implicit $rdi
+name: copyprop3
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $rdi
+
+    $rax = COPY undef $rdi
+    NOOP implicit killed $rax
+    $rax = COPY $rdi
+    NOOP implicit $rax, implicit $rdi
+...
+---
+# Check that undef is NOT removed from the remaining copy because
+# the deleted COPY also had it.
+# CHECK-LABEL: name: copyprop4
+# CHECK: bb.0:
+# CHECK:      $rax = COPY undef $rdi
+# CHECK-NEXT: NOOP implicit $rax
+# CHECK-NEXT: NOOP implicit $rax, implicit $rdi
+name: copyprop4
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $rdi
+
+    $rax = COPY undef $rdi
+    NOOP implicit killed $rax
+    $rax = COPY undef $rdi
+    NOOP implicit $rax, implicit $rdi
+...
+---
 # The second copy is not redundant if the source register ($rax) is clobbered
 # even if the dest ($rbp) is not.
 # CHECK-LABEL: name: nocopyprop0
 # CHECK: bb.0:
 # CHECK-NEXT: $rax = COPY $rbp
 # CHECK-NEXT: CALL64pcrel32 @foo, csr_64, implicit $rax, implicit $rbp
 # CHECK-NEXT: $rbp = COPY $rax
 # CHECK-NEXT: NOOP implicit $rax, implicit $rbp

 # 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
-...