[PATCH] D49425: [MemorySSAUpdater] Update Phi operands after trivial Phi elimination

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea created this revision.
labrinea added reviewers: llvm-commits, efriedma, george.burgess.iv.
Herald added a subscriber: Prazek.

Bug fix for PR37445. The regression test is a reduced version of the original reproducer attached to the bug report. The underlying problem and its fix are similar to PR37808. The bug lies in `MemorySSAUpdater::getPreviousDefRecursive()`, where `PhiOps` is computed before the call to `tryRemoveTrivialPhi()` and it ends up being out of date, pointing to stale data.


https://reviews.llvm.org/D49425

Files:
  lib/Analysis/MemorySSAUpdater.cpp
  test/Bitcode/pr37445.ll
  test/Bitcode/pr37445.ll.bc


Index: test/Bitcode/pr37445.ll
===================================================================
--- /dev/null
+++ test/Bitcode/pr37445.ll
@@ -0,0 +1,19 @@
+; RUN: opt < %s.bc -early-cse-memssa -gvn-hoist -S | FileCheck %s
+
+; Make sure opt won't crash and that this pair
+; of instructions is hoisted successfully from
+; bb45 and bb58 to bb41.
+
+;CHECK-LABEL: @func_22
+
+;CHECK:       bb41:
+;CHECK:         %tmp47 = load i32, i32* %arg1, align 4
+;CHECK:         %tmp48 = icmp eq i32 %tmp47, 0
+
+;CHECK:       bb45:
+;CHECK-NOT:     %tmp47 = load i32, i32* %arg1, align 4
+;CHECK-NOT:     %tmp48 = icmp eq i32 %tmp47, 0
+
+;CHECK:       bb58:
+;CHECK-NOT:     %tmp60 = load i32, i32* %arg1, align 4
+;CHECK-NOT:     %tmp61 = icmp eq i32 %tmp60, 0
Index: lib/Analysis/MemorySSAUpdater.cpp
===================================================================
--- lib/Analysis/MemorySSAUpdater.cpp
+++ lib/Analysis/MemorySSAUpdater.cpp
@@ -65,7 +65,7 @@
 
   if (VisitedBlocks.insert(BB).second) {
     // Mark us visited so we can detect a cycle
-    SmallVector<MemoryAccess *, 8> PhiOps;
+    SmallVector<WeakVH, 8> PhiOps;
 
     // Recurse to get the values in our predecessors for placement of a
     // potential phi node. This will insert phi nodes if we cycle in order to
@@ -76,30 +76,36 @@
     // Now try to simplify the ops to avoid placing a phi.
     // This may return null if we never created a phi yet, that's okay
     MemoryPhi *Phi = dyn_cast_or_null<MemoryPhi>(MSSA->getMemoryAccess(BB));
-    bool PHIExistsButNeedsUpdate = false;
-    // See if the existing phi operands match what we need.
-    // Unlike normal SSA, we only allow one phi node per block, so we can't just
-    // create a new one.
-    if (Phi && Phi->getNumOperands() != 0)
-      if (!std::equal(Phi->op_begin(), Phi->op_end(), PhiOps.begin())) {
-        PHIExistsButNeedsUpdate = true;
-      }
 
     // See if we can avoid the phi by simplifying it.
     auto *Result = tryRemoveTrivialPhi(Phi, PhiOps);
     // If we couldn't simplify, we may have to create a phi
     if (Result == Phi) {
       if (!Phi)
         Phi = MSSA->createMemoryPhi(BB);
 
-      // These will have been filled in by the recursive read we did above.
-      if (PHIExistsButNeedsUpdate) {
-        std::copy(PhiOps.begin(), PhiOps.end(), Phi->op_begin());
-        std::copy(pred_begin(BB), pred_end(BB), Phi->block_begin());
+      // PhiOps might be out of date.
+      unsigned i = 0;
+      for (auto *Pred : predecessors(BB)) {
+        auto *MA = dyn_cast_or_null<MemoryAccess>(PhiOps[i]);
+        if (!MA)
+          PhiOps[i] = getPreviousDefFromEnd(Pred, CachedPreviousDef);
+        ++i;
+      }
+
+      // See if the existing phi operands match what we need.
+      // Unlike normal SSA, we only allow one phi node per block, so we can't just
+      // create a new one.
+      if (Phi && Phi->getNumOperands() != 0) {
+        if (!std::equal(Phi->op_begin(), Phi->op_end(), PhiOps.begin())) {
+          // These will have been filled in by the recursive read we did above.
+          std::copy(PhiOps.begin(), PhiOps.end(), Phi->op_begin());
+          std::copy(pred_begin(BB), pred_end(BB), Phi->block_begin());
+        }
       } else {
         unsigned i = 0;
         for (auto *Pred : predecessors(BB))
-          Phi->addIncoming(PhiOps[i++], Pred);
+          Phi->addIncoming(cast<MemoryAccess>(PhiOps[i++]), Pred);
         InsertedPHIs.push_back(Phi);
       }
       Result = Phi;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D49425.155876.patch
Type: text/x-patch
Size: 3493 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180717/01b3c9c0/attachment.bin>