Revert r282168 "GVN-hoist: fix store past load dependence analysis (PR30216)"

and also the dependent r282175 "GVN-hoist: do not dereference null pointers"

It's causing compiler crashes building Harfbuzz (PR30499).

llvm-svn: 282199

diff --git a/llvm/lib/Transforms/Scalar/GVNHoist.cpp b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
index 03fd487..8b2164c 100644 (file)
--- a/llvm/lib/Transforms/Scalar/GVNHoist.cpp
+++ b/llvm/lib/Transforms/Scalar/GVNHoist.cpp
@@ -329,56 +329,49 @@ private:
     return I1DFS < I2DFS;
   }
 
-  // Return true when there are memory uses of Def in BB.
-  bool hasMemoryUseOnPath(const Instruction *NewPt, MemoryDef *Def, const BasicBlock *BB) {
-    const Instruction *OldPt = Def->getMemoryInst();
+  // Return true when there are users of Def in BB.
+  bool hasMemoryUseOnPath(MemoryAccess *Def, const BasicBlock *BB,
+                          const Instruction *OldPt) {
+    const BasicBlock *DefBB = Def->getBlock();
     const BasicBlock *OldBB = OldPt->getParent();
-    const BasicBlock *NewBB = NewPt->getParent();
 
-    bool ReachedNewPt = false;
-    MemoryLocation DefLoc = MemoryLocation::get(OldPt);
-    const MemorySSA::AccessList *Acc = MSSA->getBlockAccesses(BB);
-    if (!Acc)
-      return false;
+    for (User *U : Def->users())
+      if (auto *MU = dyn_cast<MemoryUse>(U)) {
+        // FIXME: MU->getBlock() does not get updated when we move the instruction.
+        BasicBlock *UBB = MU->getMemoryInst()->getParent();
+        // Only analyze uses in BB.
+        if (BB != UBB)
+          continue;
 
-    for (const MemoryAccess &MA : *Acc) {
-      auto *MU = dyn_cast<MemoryUse>(&MA);
-      if (!MU)
-        continue;
+        // A use in the same block as the Def is on the path.
+        if (UBB == DefBB) {
+          assert(MSSA->locallyDominates(Def, MU) && "def not dominating use");
+          return true;
+        }
 
-      // Do not check whether MU aliases Def when MU occurs after OldPt.
-      if (BB == OldBB && firstInBB(OldPt, MU->getMemoryInst()))
-        break;
+        if (UBB != OldBB)
+          return true;
 
-      // Do not check whether MU aliases Def when MU occurs before NewPt.
-      if (BB == NewBB) {
-        if (!ReachedNewPt) {
-          if (firstInBB(MU->getMemoryInst(), NewPt))
-            continue;
-          ReachedNewPt = true;
-        }
+        // It is only harmful to hoist when the use is before OldPt.
+        if (firstInBB(MU->getMemoryInst(), OldPt))
+          return true;
       }
 
-      if (!AA->isNoAlias(DefLoc, MemoryLocation::get(MU->getMemoryInst())))
-        return true;
-    }
-
     return false;
   }
 
   // Return true when there are exception handling or loads of memory Def
-  // between Def and NewPt.  This function is only called for stores: Def is
-  // the MemoryDef of the store to be hoisted.
+  // between OldPt and NewPt.
 
   // Decrement by 1 NBBsOnAllPaths for each block between HoistPt and BB, and
   // return true when the counter NBBsOnAllPaths reaces 0, except when it is
   // initialized to -1 which is unlimited.
-  bool hasEHOrLoadsOnPath(const Instruction *NewPt, MemoryDef *Def,
-                          int &NBBsOnAllPaths) {
+  bool hasEHOrLoadsOnPath(const Instruction *NewPt, const Instruction *OldPt,
+                          MemoryAccess *Def, int &NBBsOnAllPaths) {
     const BasicBlock *NewBB = NewPt->getParent();
-    const BasicBlock *OldBB = Def->getBlock();
+    const BasicBlock *OldBB = OldPt->getParent();
     assert(DT->dominates(NewBB, OldBB) && "invalid path");
-    assert(DT->dominates(Def->getDefiningAccess()->getBlock(), NewBB) &&
+    assert(DT->dominates(Def->getBlock(), NewBB) &&
            "def does not dominate new hoisting point");
 
     // Walk all basic blocks reachable in depth-first iteration on the inverse
@@ -397,7 +390,7 @@ private:
         return true;
 
       // Check that we do not move a store past loads.
-      if (hasMemoryUseOnPath(NewPt, Def, *I))
+      if (hasMemoryUseOnPath(Def, *I, OldPt))
         return true;
 
       // Stop walk once the limit is reached.
@@ -480,7 +473,7 @@ private:
 
     // Check for unsafe hoistings due to side effects.
     if (K == InsKind::Store) {
-      if (hasEHOrLoadsOnPath(NewPt, dyn_cast<MemoryDef>(U), NBBsOnAllPaths))
+      if (hasEHOrLoadsOnPath(NewPt, OldPt, D, NBBsOnAllPaths))
         return false;
     } else if (hasEHOnPath(NewBB, OldBB, NBBsOnAllPaths))
       return false;

diff --git a/llvm/test/Transforms/GVNHoist/pr30216.ll b/llvm/test/Transforms/GVNHoist/pr30216.ll
deleted file mode 100644 (file)
index b2ce338..0000000
--- a/llvm/test/Transforms/GVNHoist/pr30216.ll
+++ /dev/null
@@ -1,52 +0,0 @@
-; RUN: opt -S -gvn-hoist < %s | FileCheck %s
-
-; Make sure the two stores @B do not get hoisted past the load @B.
-
-; CHECK-LABEL: define i8* @Foo
-; CHECK: store
-; CHECK: store
-; CHECK: load
-; CHECK: store
-
-@A = external global i8
-@B = external global i8*
-
-define i8* @Foo() {
-  store i8 0, i8* @A
-  br i1 undef, label %if.then, label %if.else
-
-if.then:
-  store i8* null, i8** @B
-  ret i8* null
-
-if.else:
-  %1 = load i8*, i8** @B
-  store i8* null, i8** @B
-  ret i8* %1
-}
-
-; Make sure the two stores @B do not get hoisted past the store @GlobalVar.
-
-; CHECK-LABEL: define i8* @Fun
-; CHECK: store
-; CHECK: store
-; CHECK: store
-; CHECK: store
-; CHECK: load
-
-@GlobalVar = internal global i8 0
-
-define i8* @Fun() {
-  store i8 0, i8* @A
-  br i1 undef, label %if.then, label %if.else
-
-if.then:
-  store i8* null, i8** @B
-  ret i8* null
-
-if.else:
-  store i8 0, i8* @GlobalVar
-  store i8* null, i8** @B
-  %1 = load i8*, i8** @B
-  ret i8* %1
-}
\ No newline at end of file