MachineCopyPropagation: Do not remove copies preserved by regmask

[jsji]

9e436c2daa44 tries to handle register masks and
sub-registers, it avoids clobbering RegUnit presreved by regmask. But it
then introduces invalid pointer issues.

We delete the copies without invalidate all the use in the CopyInfo, so
we dereferenced invalid pointers in next interation, causing asserts.

Fixes: #126107

[llvmbot]

@llvm/pr-subscribers-llvm-regalloc

Author: Jinsong Ji (jsji)

Changes

llvm/llvm-project@9e436c2 tries to handle register masks and
sub-registers, it avoids clobbering RegUnit presreved by regmask. But it
then introduces invalid pointer issues.

We delete the copies without invalidate all the use in the CopyInfo, so
we dereferenced invalid pointers in next interation, causing asserts.

---

Full diff: https://github.com/llvm/llvm-project/pull/125868.diff

1 Files Affected:

• (modified) llvm/lib/CodeGen/MachineCopyPropagation.cpp (+16-4)

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 460749a739c763..ba63d4c69d0900 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -1018,18 +1018,30 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
           continue;
         }
 
-        LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: ";
-                   MaybeDead->dump());
-
         // Invalidate all entries in the copy map which are not preserved by
         // this register mask.
-        for (unsigned RegUnit : TRI->regunits(Reg))
+        bool MIRefedinCopyInfo = false;
+        for (unsigned RegUnit : TRI->regunits(Reg)) {
           if (!PreservedRegUnits.test(RegUnit))
             Tracker.clobberRegUnit(RegUnit, *TRI, *TII, UseCopyInstr);
+          else {
+            if (MaybeDead == Tracker.findCopyForUnit(RegUnit, *TRI)) {
+              MIRefedinCopyInfo = true;
+            }
+          }
+        }
 
         // erase() will return the next valid iterator pointing to the next
         // element after the erased one.
         DI = MaybeDeadCopies.erase(DI);
+
+        // Preserved by RegMask, DO NOT remove copy
+        if (MIRefedinCopyInfo)
+          continue;
+
+        LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: ";
+                   MaybeDead->dump());
+
         MaybeDead->eraseFromParent();
         Changed = true;
         ++NumDeletes;

[bzEq]

Could you please provide the test causing assertion failure? MIR test is preferred.
Ok, I have read the comments in the previous PR.

[JaydeepChauhan14]

@arsenm , @evanphx , @bzEq can please review pr.

[ostannard]

Issue #126107 has a reproducer for this, so that can be added as a test. I've checked that applying this patch does fix that assertion.

[jsji]

Issue #126107 has a reproducer for this, so that can be added as a test. I've checked that applying this patch does fix that assertion.

Thanks! Added MIR test from #126107.

[ostannard]

Thanks, LGTM

[ostannard]

Missing close-paren, which is causing the build failure.

Suggested change

	LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: " << *MaybeDead;
	LLVM_DEBUG(dbgs() << "MCP: Removing copy due to regmask clobbering: " << *MaybeDead);