From: Sebastian Pop Date: Thu, 22 Sep 2016 15:33:51 +0000 (+0000) Subject: GVN-hoist: fix store past load dependence analysis (PR30216) X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=3e55f3d02314b02ad2216dbd08295985c5576a11;p=llvm GVN-hoist: fix store past load dependence analysis (PR30216) To hoist stores past loads, we used to search for potential conflicting loads on the hoisting path by following a MemorySSA def-def link from the store to be hoisted to the previous defining memory access, and from there we followed the def-use chains to all the uses that occur on the hoisting path. The problem is that the def-def link may point to a store that does not alias with the store to be hoisted, and so the loads that are walked may not alias with the store to be hoisted, and even as in the testcase of PR30216, the loads that may alias with the store to be hoisted are not visited. The current patch visits all loads on the path from the store to be hoisted to the hoisting position and uses the alias analysis to ask whether the store may alias the load. I was not able to use the MemorySSA functionality to ask for whether load and store are clobbered: I'm not sure which function to call, so I used a call to AA->isNoAlias(). Store past store is still working as before using a MemorySSA query: I added an extra test to pr30216.ll to make sure store past store does not regress. Differential Revision: https://reviews.llvm.org/D24517 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@282168 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/Transforms/Scalar/GVNHoist.cpp b/lib/Transforms/Scalar/GVNHoist.cpp index 8b2164c5071..3c2ac6fcd5f 100644 --- a/lib/Transforms/Scalar/GVNHoist.cpp +++ b/lib/Transforms/Scalar/GVNHoist.cpp @@ -329,49 +329,53 @@ private: return I1DFS < I2DFS; } - // 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(); + // 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(); const BasicBlock *OldBB = OldPt->getParent(); + const BasicBlock *NewBB = NewPt->getParent(); - for (User *U : Def->users()) - if (auto *MU = dyn_cast(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; - } + bool ReachedNewPt = false; + MemoryLocation DefLoc = MemoryLocation::get(OldPt); + const MemorySSA::AccessList *Acc = MSSA->getBlockAccesses(BB); + for (const MemoryAccess &MA : *Acc) { + auto *MU = dyn_cast(&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; - // It is only harmful to hoist when the use is before OldPt. - if (firstInBB(MU->getMemoryInst(), OldPt)) - 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; + } } + 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 OldPt and NewPt. + // between Def and NewPt. This function is only called for stores: Def is + // the MemoryDef of the store to be hoisted. // 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, const Instruction *OldPt, - MemoryAccess *Def, int &NBBsOnAllPaths) { + bool hasEHOrLoadsOnPath(const Instruction *NewPt, MemoryDef *Def, + int &NBBsOnAllPaths) { const BasicBlock *NewBB = NewPt->getParent(); - const BasicBlock *OldBB = OldPt->getParent(); + const BasicBlock *OldBB = Def->getBlock(); assert(DT->dominates(NewBB, OldBB) && "invalid path"); - assert(DT->dominates(Def->getBlock(), NewBB) && + assert(DT->dominates(Def->getDefiningAccess()->getBlock(), NewBB) && "def does not dominate new hoisting point"); // Walk all basic blocks reachable in depth-first iteration on the inverse @@ -390,7 +394,7 @@ private: return true; // Check that we do not move a store past loads. - if (hasMemoryUseOnPath(Def, *I, OldPt)) + if (hasMemoryUseOnPath(NewPt, Def, *I)) return true; // Stop walk once the limit is reached. @@ -473,7 +477,7 @@ private: // Check for unsafe hoistings due to side effects. if (K == InsKind::Store) { - if (hasEHOrLoadsOnPath(NewPt, OldPt, D, NBBsOnAllPaths)) + if (hasEHOrLoadsOnPath(NewPt, dyn_cast(U), NBBsOnAllPaths)) return false; } else if (hasEHOnPath(NewBB, OldBB, NBBsOnAllPaths)) return false; diff --git a/test/Transforms/GVNHoist/pr30216.ll b/test/Transforms/GVNHoist/pr30216.ll new file mode 100644 index 00000000000..b2ce338f8ae --- /dev/null +++ b/test/Transforms/GVNHoist/pr30216.ll @@ -0,0 +1,52 @@ +; 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