[llvm] r282199 - Revert r282168 "GVN-hoist: fix store past load dependence analysis (PR30216)"
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 22 14:20:53 PDT 2016
Author: hans
Date: Thu Sep 22 16:20:53 2016
New Revision: 282199
URL: http://llvm.org/viewvc/llvm-project?rev=282199&view=rev
Log:
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).
Removed:
llvm/trunk/test/Transforms/GVNHoist/pr30216.ll
Modified:
llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp?rev=282199&r1=282198&r2=282199&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Thu Sep 22 16:20:53 2016
@@ -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;
+
+ // 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;
+ }
- for (const MemoryAccess &MA : *Acc) {
- auto *MU = dyn_cast<MemoryUse>(&MA);
- if (!MU)
- continue;
+ if (UBB != OldBB)
+ return true;
- // Do not check whether MU aliases Def when MU occurs after OldPt.
- if (BB == OldBB && firstInBB(OldPt, MU->getMemoryInst()))
- break;
-
- // 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;
Removed: llvm/trunk/test/Transforms/GVNHoist/pr30216.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVNHoist/pr30216.ll?rev=282198&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/GVNHoist/pr30216.ll (original)
+++ llvm/trunk/test/Transforms/GVNHoist/pr30216.ll (removed)
@@ -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
-
- at A = external global i8
- at 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
-
- at 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
More information about the llvm-commits
mailing list