[PATCH] D21041: [GVN] PRE can't hoist loads across calls in general.
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 13 14:56:19 PDT 2016
-reviews because i'm attaching a patch
If we aren't going to tackle the broader architectural change now, i'm okay
with doing something here.
I've attached what i did to mergedloadstoremotion's N^2 algorithm, which is
only cares about per-block answers as well[1]
If you need to see if you can hoist to a generic block above you in the
dominator tree, you'd need in/out numbers, and can duplicate what we do for
the dom tree itself when comparing the DFS Numbers.
But i think, as you've discovered and i mentioned in another thread, most
things only care about whether a given block has maythrow instructions.
Given how much you are going to discover is going to break here in funny
ways without it being part of the CFG, i think we should just make the CFG
look right, and have fake EH edges. Anything else is going to be a true
mess over time.
[1]
What i've done is also equivalent to just making OrderedBlock into
OrderedBlocks, without the laziness. If you wanted that lazy abstraction,
it's harder but doable.
On Sun, Jun 12, 2016 at 1:00 PM, Eli Friedman <eli.friedman at gmail.com>
wrote:
> eli.friedman updated this revision to Diff 60478.
> eli.friedman added a comment.
>
> Improved control-flow check, fix for scalar PRE.
>
> Still has crappy O(N^2) loop. My current thinking is that it's possible
> to precompute it on a
> per-block basis in GVN::runImpl.
>
>
> http://reviews.llvm.org/D21041
>
> Files:
> lib/Transforms/Scalar/GVN.cpp
> test/Transforms/GVN/local-pre.ll
> test/Transforms/GVN/pre-load.ll
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160613/f525d0f3/attachment.html>
-------------- next part --------------
diff --git a/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp b/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
index 22c89be..0e6bd0b 100644
--- a/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
+++ b/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
@@ -88,8 +88,8 @@
#include "llvm/Transforms/Utils/BasicBlockUtils.h"
#include "llvm/Transforms/Utils/MemorySSA.h"
#include <queue>
-#include <vector>
#include <unordered_set>
+#include <vector>
using namespace llvm;
@@ -156,6 +156,10 @@ class MergedLoadStoreMotion : public FunctionPass {
// sides of the diamond. The constant chosen here is arbitrary. Compiler Time
// Control is enforced by the check Size0 * Size1 < MagicCompileTimeControl.
const int MagicCompileTimeControl;
+ // We DFS number instructions and avoid hoisting things past may throw
+ // instructions.
+ DenseMap<const Instruction *, unsigned> DFSNumberMap;
+ DenseMap<const BasicBlock *, unsigned> FirstMayThrowMap;
public:
static char ID; // Pass identification, replacement for typeid
@@ -191,8 +195,7 @@ private:
bool isDiamondHead(BasicBlock *BB);
// Routines for hoisting loads
bool isLoadHoistBarrierInRange(const Instruction &Start,
- const Instruction &End, LoadInst *LI,
- bool SafeToLoadUnconditionally);
+ const Instruction &End, LoadInst *LI);
LoadInst *canHoistFromBlock(BasicBlock *BB, LoadInst *LI);
void hoistInstruction(BasicBlock *BB, Instruction *HoistCand,
Instruction *ElseInst);
@@ -207,11 +210,10 @@ private:
const Instruction &End, MemoryLocation Loc);
bool sinkStore(BasicBlock *BB, StoreInst *SinkCand, StoreInst *ElseInst);
bool mergeStores(BasicBlock *BB);
-
+ void DFSNumberBlocks();
std::unordered_multiset<LoadPair, LoadPairHash, LoadPairEq> LoadInfo;
std::unordered_multiset<StoreInst *, StoreInstHash, StoreInstEq> StoreInfo;
SmallPtrSet<MemoryAccess *, 8> AccessesToDelete;
- DenseMap<const BasicBlock *, std::pair<int, int>> DFSDomMap;
};
char MergedLoadStoreMotion::ID = 0;
@@ -289,14 +291,9 @@ bool MergedLoadStoreMotion::isDiamondHead(BasicBlock *BB) {
/// being loaded or protect against the load from happening
/// it is considered a hoist barrier.
///
-bool MergedLoadStoreMotion::isLoadHoistBarrierInRange(
- const Instruction &Start, const Instruction &End, LoadInst *LI,
- bool SafeToLoadUnconditionally) {
- if (!SafeToLoadUnconditionally)
- for (const Instruction &Inst :
- make_range(Start.getIterator(), End.getIterator()))
- if (Inst.mayThrow())
- return true;
+bool MergedLoadStoreMotion::isLoadHoistBarrierInRange(const Instruction &Start,
+ const Instruction &End,
+ LoadInst *LI) {
MemoryLocation Loc = MemoryLocation::get(LI);
return AA->canInstructionRangeModRef(Start, End, Loc, MRI_Mod);
}
@@ -327,11 +324,22 @@ LoadInst *MergedLoadStoreMotion::canHoistFromBlock(BasicBlock *BB1,
MemoryLocation Loc0 = MemoryLocation::get(Load0);
MemoryLocation Loc1 = MemoryLocation::get(Load1);
+ if (!SafeToLoadUnconditionally) {
+ unsigned int BB0MayThrow = FirstMayThrowMap.lookup(BB0);
+ // If the first maythrow instruction in the block is before us, we can't
+ // hoist
+ if (BB0MayThrow != 0 && DFSNumberMap.lookup(Load0) > BB0MayThrow)
+ continue;
+ unsigned int BB1MayThrow = FirstMayThrowMap.lookup(BB1);
+ // If the first maythrow instruction in the block is before us, we can't
+ // hoist
+ if (BB1MayThrow != 0 && DFSNumberMap.lookup(Load1) > BB1MayThrow)
+ continue;
+ }
+
if (AA->isMustAlias(Loc0, Loc1) && Load0->isSameOperationAs(Load1) &&
- !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1,
- SafeToLoadUnconditionally) &&
- !isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0,
- SafeToLoadUnconditionally)) {
+ !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1) &&
+ !isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0)) {
return Load1;
}
}
@@ -711,6 +719,26 @@ bool MergedLoadStoreMotion::mergeStores(BasicBlock *T) {
return MergedStores;
}
+// \brief DFS Number instructions in blocks in dominator order
+void MergedLoadStoreMotion::DFSNumberBlocks() {
+
+ // This is 1 so the default constructed value of 0 can be used to say we
+ // didn't find anything
+ unsigned DFSNum = 1;
+ for (auto DI = df_begin(DT->getRootNode()), DE = df_end(DT->getRootNode());
+ DI != DE; ++DI) {
+ bool FoundMayThrowInBlock = false;
+ for (const auto &Inst : *(DI->getBlock())) {
+ DFSNumberMap[&Inst] = DFSNum;
+ if (!FoundMayThrowInBlock && Inst.mayThrow()) {
+ FirstMayThrowMap[DI->getBlock()] = DFSNum;
+ FoundMayThrowInBlock = true;
+ }
+ ++DFSNum;
+ }
+ }
+}
+
///
/// \brief Run the transformation for each function
///
@@ -721,15 +749,15 @@ bool MergedLoadStoreMotion::runOnFunction(Function &F) {
auto *MDWP = getAnalysisIfAvailable<MemoryDependenceWrapperPass>();
MD = MDWP ? &MDWP->getMemDep() : nullptr;
AA = &getAnalysis<AAResultsWrapperPass>().getAAResults();
+ DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
if (UseMemorySSA) {
- DT = &getAnalysis<DominatorTreeWrapperPass>().getDomTree();
MSSA = &getAnalysis<MemorySSAWrapperPass>().getMSSA();
CachingWalker = MSSA->getWalker();
}
bool Changed = false;
DEBUG(dbgs() << "Instruction Merger\n");
-
+ DFSNumberBlocks();
// Merge unconditional branches, allowing PRE to catch more
// optimization opportunities.
for (BasicBlock &BB : F) {
More information about the llvm-commits
mailing list