[PATCH] D122601: Fix MemorySSAUpdater::insertDef for dead code

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko created this revision.
apilipenko added reviewers: george.burgess.iv, asbirlea, fhahn.
Herald added a subscriber: hiraditya.
Herald added a project: All.
apilipenko requested review of this revision.
Herald added a project: LLVM.

Currently, the attached test case fails with an assert in MemorySSAUpdater::fixupDefs:

  // We are now this def's defining access, make sure we actually dominate
  // it
  assert(MSSA->dominates(NewDef, FirstDef) &&
         "Should have dominated the new access");

See https://github.com/llvm/llvm-project/issues/51257

This happens because we are trying to insert a def in dead code. In this test case GVN attempts to merge baz.invoke block into dead.block (both blocks are dead, i.e. unreachable from entry). This triggers an update to MemorySSA with the following call chain:

  MergeBlockIntoPredecessor
    MemorySSAUpdater::moveToPlace
      MemorySSAUpdater::moveTo
        MemorySSAUpdater::insertDef
          MemorySSAUpdater::fixupDefs

fixupDefs asserts that the new defining access dominates the memory access being updated. This assert fails for dead code because DomTree doesn't provide dominance information for blocks not reachable from entry (it simply returns false).

When constructing a fresh MSSA we mark all memory accesses in dead code as live-on-entry. In this patch, I do the same for incremental updates via insertDef.


https://reviews.llvm.org/D122601

Files:
  llvm/lib/Analysis/MemorySSAUpdater.cpp
  llvm/test/Analysis/MemorySSA/update-dead-def.ll


Index: llvm/test/Analysis/MemorySSA/update-dead-def.ll
===================================================================
--- /dev/null
+++ llvm/test/Analysis/MemorySSA/update-dead-def.ll
@@ -0,0 +1,30 @@
+; RUN: opt -passes='require<memoryssa>,gvn' -verify-memoryssa -S %s | FileCheck %s
+
+; CHECK: @test()
+define void @test() personality i32* ()* null {
+  invoke void @bar()
+          to label %bar.normal unwind label %exceptional
+
+bar.normal:
+  ret void
+
+dead.block:
+  br label %baz.invoke
+
+baz.invoke:
+  invoke void @baz()
+          to label %baz.normal unwind label %exceptional
+
+baz.normal:
+  ret void
+
+exceptional:
+  %tmp9 = landingpad { i8*, i32 }
+          cleanup
+  call void @foo()
+  ret void
+}
+
+declare void @foo()
+declare void @bar()
+declare void @baz()
Index: llvm/lib/Analysis/MemorySSAUpdater.cpp
===================================================================
--- llvm/lib/Analysis/MemorySSAUpdater.cpp
+++ llvm/lib/Analysis/MemorySSAUpdater.cpp
@@ -305,6 +305,12 @@
 // point to the correct new defs, to ensure we only have one variable, and no
 // disconnected stores.
 void MemorySSAUpdater::insertDef(MemoryDef *MD, bool RenameUses) {
+  // Don't bother updating dead code.
+  if (!MSSA->DT->isReachableFromEntry(MD->getBlock())) {
+    MD->setDefiningAccess(MSSA->getLiveOnEntryDef());
+    return;
+  }
+
   VisitedBlocks.clear();
   InsertedPHIs.clear();
 
@@ -422,10 +428,10 @@
   if (NewPhiSize)
     tryRemoveTrivialPhis(ArrayRef<WeakVH>(&InsertedPHIs[NewPhiIndex], NewPhiSize));
 
-  // Now that all fixups are done, rename all uses if we are asked. Skip
-  // renaming for defs in unreachable blocks.
+  // Now that all fixups are done, rename all uses if we are asked. The defs are
+  // guaranteed to be in reachable code due to the check at the method entry.
   BasicBlock *StartBlock = MD->getBlock();
-  if (RenameUses && MSSA->getDomTree().getNode(StartBlock)) {
+  if (RenameUses) {
     SmallPtrSet<BasicBlock *, 16> Visited;
     // We are guaranteed there is a def in the block, because we just got it
     // handed to us in this function.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D122601.418614.patch
Type: text/x-patch
Size: 2114 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20220328/afd58a35/attachment.bin>