[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 15:02:02 PDT 2016
Whoops, wrong version.
Here's the one that does both loads and stores
On Mon, Jun 13, 2016 at 2:56 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> -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/f30d6ce2/attachment.html>
-------------- next part --------------
diff --git a/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp b/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
index 22c89be..50bcd3d 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,11 @@ 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;
+ DenseMap<const BasicBlock *, unsigned> LastMayThrowMap;
public:
static char ID; // Pass identification, replacement for typeid
@@ -191,8 +196,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 +211,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 +292,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 +325,20 @@ LoadInst *MergedLoadStoreMotion::canHoistFromBlock(BasicBlock *BB1,
MemoryLocation Loc0 = MemoryLocation::get(Load0);
MemoryLocation Loc1 = MemoryLocation::get(Load1);
+ if (!SafeToLoadUnconditionally) {
+ // If the first maythrow instruction in the block is before the load, we
+ // can't hoist.
+ unsigned int BB0MayThrow = FirstMayThrowMap.lookup(BB0);
+ if (BB0MayThrow != 0 && DFSNumberMap.lookup(Load0) > BB0MayThrow)
+ continue;
+ unsigned int BB1MayThrow = FirstMayThrowMap.lookup(BB1);
+ 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;
}
}
@@ -553,10 +560,6 @@ bool MergedLoadStoreMotion::mergeLoads(BasicBlock *BB) {
bool MergedLoadStoreMotion::isStoreSinkBarrierInRange(const Instruction &Start,
const Instruction &End,
MemoryLocation Loc) {
- for (const Instruction &Inst :
- make_range(Start.getIterator(), End.getIterator()))
- if (Inst.mayThrow())
- return true;
return AA->canInstructionRangeModRef(Start, End, Loc, MRI_ModRef);
}
@@ -579,6 +582,15 @@ StoreInst *MergedLoadStoreMotion::canSinkFromBlock(BasicBlock *BB1,
MemoryLocation Loc0 = MemoryLocation::get(Store0);
MemoryLocation Loc1 = MemoryLocation::get(Store1);
+ // If the last maythrow instruction the block is after us, we can't sink out
+ // of the block.
+ unsigned int BB0MayThrow = LastMayThrowMap.lookup(BB0);
+ if (BB0MayThrow != 0 && DFSNumberMap.lookup(Store0) < BB0MayThrow)
+ continue;
+ unsigned int BB1MayThrow = LastMayThrowMap.lookup(BB1);
+ if (BB1MayThrow != 0 && DFSNumberMap.lookup(Store1) < BB1MayThrow)
+ continue;
+
if (AA->isMustAlias(Loc0, Loc1) && Store0->isSameOperationAs(Store1) &&
!isStoreSinkBarrierInRange(*Store1->getNextNode(), BB1->back(), Loc1) &&
!isStoreSinkBarrierInRange(*Store0->getNextNode(), BB0->back(), Loc0)) {
@@ -711,6 +723,30 @@ 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) {
+ BasicBlock *DomBlock = DI->getBlock();
+ bool FoundMayThrowInBlock = false;
+ for (const auto &Inst : *DomBlock) {
+ DFSNumberMap[&Inst] = DFSNum;
+ if (Inst.mayThrow()) {
+ if (!FoundMayThrowInBlock){
+ FirstMayThrowMap[DomBlock] = DFSNum;
+ FoundMayThrowInBlock = true;
+ }
+ LastMayThrowMap[DomBlock] = DFSNum;
+ }
+ ++DFSNum;
+ }
+ }
+}
+
///
/// \brief Run the transformation for each function
///
@@ -721,15 +757,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