[llvm] r337149 - [MemorySSAUpdater] Remove deleted trivial Phis from active workset

[Alexandros Lamprineas via llvm-commits]

Author: alelab01
Date: Mon Jul 16 00:51:27 2018
New Revision: 337149

URL: http://llvm.org/viewvc/llvm-project?rev=337149&view=rev
Log:
[MemorySSAUpdater] Remove deleted trivial Phis from active workset

Bug fix for PR37808. The regression test is a reduced version of the
original reproducer attached to the bug report. As stated in the report,
the problem was that InsertedPHIs was keeping dangling pointers to
deleted Memory-Phis. MemoryPhis are created eagerly and sometimes get
zapped shortly afterwards. I've used WeakVH instead of an expensive
removal operation from the active workset.

Differential Revision: https://reviews.llvm.org/D48372

Added:
    llvm/trunk/test/Transforms/GVNHoist/pr37808.ll
Modified:
    llvm/trunk/include/llvm/Analysis/MemorySSAUpdater.h
    llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp

Modified: llvm/trunk/include/llvm/Analysis/MemorySSAUpdater.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/MemorySSAUpdater.h?rev=337149&r1=337148&r2=337149&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Analysis/MemorySSAUpdater.h (original)
+++ llvm/trunk/include/llvm/Analysis/MemorySSAUpdater.h Mon Jul 16 00:51:27 2018
@@ -60,7 +60,11 @@ class raw_ostream;
 class MemorySSAUpdater {
 private:
   MemorySSA *MSSA;
-  SmallVector<MemoryPhi *, 8> InsertedPHIs;
+
+  /// We use WeakVH rather than a costly deletion to deal with dangling pointers.
+  /// MemoryPhis are created eagerly and sometimes get zapped shortly afterwards.
+  SmallVector<WeakVH, 16> InsertedPHIs;
+
   SmallPtrSet<BasicBlock *, 8> VisitedBlocks;
   SmallSet<AssertingVH<MemoryPhi>, 8> NonOptPhis;
 
@@ -207,7 +211,7 @@ private:
   MemoryAccess *recursePhi(MemoryAccess *Phi);
   template <class RangeType>
   MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi, RangeType &Operands);
-  void fixupDefs(const SmallVectorImpl<MemoryAccess *> &);
+  void fixupDefs(const SmallVectorImpl<WeakVH> &);
 };
 } // end namespace llvm
 

Modified: llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp?rev=337149&r1=337148&r2=337149&view=diff
==============================================================================
--- llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp (original)
+++ llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp Mon Jul 16 00:51:27 2018
@@ -278,8 +278,7 @@ void MemorySSAUpdater::insertDef(MemoryD
   // above and reset ourselves.
   MD->setDefiningAccess(DefBefore);
 
-  SmallVector<MemoryAccess *, 8> FixupList(InsertedPHIs.begin(),
-                                           InsertedPHIs.end());
+  SmallVector<WeakVH, 8> FixupList(InsertedPHIs.begin(), InsertedPHIs.end());
   if (!DefBeforeSameBlock) {
     // If there was a local def before us, we must have the same effect it
     // did. Because every may-def is the same, any phis/etc we would create, it
@@ -300,7 +299,7 @@ void MemorySSAUpdater::insertDef(MemoryD
     fixupDefs(FixupList);
     FixupList.clear();
     // Put any new phis on the fixup list, and process them
-    FixupList.append(InsertedPHIs.end() - StartingPHISize, InsertedPHIs.end());
+    FixupList.append(InsertedPHIs.begin() + StartingPHISize, InsertedPHIs.end());
   }
   // Now that all fixups are done, rename all uses if we are asked.
   if (RenameUses) {
@@ -317,15 +316,21 @@ void MemorySSAUpdater::insertDef(MemoryD
     MSSA->renamePass(MD->getBlock(), FirstDef, Visited);
     // We just inserted a phi into this block, so the incoming value will become
     // the phi anyway, so it does not matter what we pass.
-    for (auto *MP : InsertedPHIs)
-      MSSA->renamePass(MP->getBlock(), nullptr, Visited);
+    for (auto &MP : InsertedPHIs) {
+      MemoryPhi *Phi = dyn_cast_or_null<MemoryPhi>(MP);
+      if (Phi)
+        MSSA->renamePass(Phi->getBlock(), nullptr, Visited);
+    }
   }
 }
 
-void MemorySSAUpdater::fixupDefs(const SmallVectorImpl<MemoryAccess *> &Vars) {
+void MemorySSAUpdater::fixupDefs(const SmallVectorImpl<WeakVH> &Vars) {
   SmallPtrSet<const BasicBlock *, 8> Seen;
   SmallVector<const BasicBlock *, 16> Worklist;
-  for (auto *NewDef : Vars) {
+  for (auto &Var : Vars) {
+    MemoryAccess *NewDef = dyn_cast_or_null<MemoryAccess>(Var);
+    if (!NewDef)
+      continue;
     // First, see if there is a local def after the operand.
     auto *Defs = MSSA->getWritableBlockDefs(NewDef->getBlock());
     auto DefIter = NewDef->getDefsIterator();

Added: llvm/trunk/test/Transforms/GVNHoist/pr37808.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVNHoist/pr37808.ll?rev=337149&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/GVNHoist/pr37808.ll (added)
+++ llvm/trunk/test/Transforms/GVNHoist/pr37808.ll Mon Jul 16 00:51:27 2018
@@ -0,0 +1,40 @@
+; RUN: opt < %s -gvn-hoist -S | FileCheck %s
+
+define void @func() {
+; CHECK-LABEL: @func()
+; CHECK:       bb6:
+; CHECK:         store i64 0, i64* undef, align 8
+; CHECK:       bb7:
+; CHECK-NOT:     store i64 0, i64* undef, align 8
+; CHECK:       bb8:
+; CHECK-NOT:     store i64 0, i64* undef, align 8
+
+entry:
+  br label %bb1
+
+bb1:
+  br label %bb2
+
+bb2:
+  br label %bb3
+
+bb3:
+  br i1 undef, label %bb4, label %bb2
+
+bb4:
+  br i1 undef, label %bb5, label %bb3
+
+bb5:
+  br label %bb6
+
+bb6:
+  br i1 undef, label %bb7, label %bb8
+
+bb7:
+  store i64 0, i64* undef, align 8
+  unreachable
+
+bb8:
+  store i64 0, i64* undef, align 8
+  ret void
+}