[RegisterCoalescer] Fix reuse of instruction pointers

[vvuksanovic]

If a pointer gets recycled while it is already in a work list, it can cause duplicates in the work lists. In that case the same instruction may be processed twice. If it is coalesced the first time, processing it again would read deallocated memory. This is fixed by removing the first occurrence from the work lists when we detect a pointer was recycled.

Fixes #71178

[llvmbot]

@llvm/pr-subscribers-llvm-regalloc

Author: Vladimir Vuksanovic (vvuksanovic)

Changes

If a pointer gets recycled while it is already in a work list, it can cause duplicates in the work lists. In that case the same instruction may be processed twice. If it is coalesced the first time, processing it again would read deallocated memory. This is fixed by removing the first occurrence from the work lists when we detect a pointer was recycled.

Fixes #71178

---

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

1 Files Affected:

• (modified) llvm/lib/CodeGen/RegisterCoalescer.cpp (+22-1)

diff --git a/llvm/lib/CodeGen/RegisterCoalescer.cpp b/llvm/lib/CodeGen/RegisterCoalescer.cpp
index 9858482cd51b4a7..a68ebaa9b6828a4 100644
--- a/llvm/lib/CodeGen/RegisterCoalescer.cpp
+++ b/llvm/lib/CodeGen/RegisterCoalescer.cpp
@@ -1031,6 +1031,19 @@ RegisterCoalescer::removeCopyByCommutingDef(const CoalescerPair &CP,
   return { true, ShrinkB };
 }
 
+// Replace the given MachineInstr * with nullptr inside a work list. Used to
+// remove an instruction when its pointer is reused. Returns true if replaced,
+// false otherwise.
+static bool removeFromWorkList(MutableArrayRef<MachineInstr *> WorkList,
+                               MachineInstr *MI) {
+  auto It = std::find(WorkList.begin(), WorkList.end(), MI);
+  if (It != WorkList.end()) {
+    *It = nullptr;
+    return true;
+  }
+  return false;
+}
+
 /// For copy B = A in BB2, if A is defined by A = B in BB0 which is a
 /// predecessor of BB2, and if B is not redefined on the way from A = B
 /// in BB0 to B = A in BB2, B = A in BB2 is partially redundant if the
@@ -1190,7 +1203,15 @@ bool RegisterCoalescer::removePartialRedundancy(const CoalescerPair &CP,
     // If the newly created Instruction has an address of an instruction that was
     // deleted before (object recycled by the allocator) it needs to be removed from
     // the deleted list.
-    ErasedInstrs.erase(NewCopyMI);
+    bool WasErased = ErasedInstrs.erase(NewCopyMI);
+    // Also remove the deleted instruction from work lists. There shouldn't be
+    // duplicate instructions there.
+    if (WasErased) {
+      // Attempt to remove from WorkList. If not found, it could be in
+      // LocalWorkList.
+      if (!removeFromWorkList(WorkList, NewCopyMI))
+        removeFromWorkList(LocalWorkList, NewCopyMI);
+    }
   } else {
     LLVM_DEBUG(dbgs() << "\tremovePartialRedundancy: Remove the copy from "
                       << printMBBReference(MBB) << '\t' << CopyMI);

[arsenm]

Needs test.

I'm not a fan of try to remove from the worklist after the fact, and then try the second worklist. We know from the copy which list it was in, whether it was local or not.

I've also never trusted this managing pointers to instructions that have already been deleted scheme.

[arsenm]

For the partial redundancy case, can't it only be in the global worklist?

[arsenm]

I don't really trust this system of relying on deleted instruction pointers. Is there a better way to handle this?

[DianQK]

Hi, are you continuing to work at this PR? Even though I haven't seen any other activity from you, I should still ask. :)

I would want you to go ahead and finish if you can. Because I see this is your first PR.