[llvm] r270828 - [MergedLoadStoreMotion] Don't transform across may-throw calls
David Majnemer via llvm-commits
llvm-commits at lists.llvm.org
Thu May 26 16:25:52 PDT 2016
On Thu, May 26, 2016 at 4:17 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
> This seems ... problematic, in a simple sense.
>
> You've added a check that literally every instruction in between does not
> throw :)
>
> It happens that the alias check is O(N) for memdep so it was all N^2
> anyway, but for memoryssa it's O(1), and you've now made such a walk N^2 in
> those case.
> This whole check should be in the outer loop of mergeLoads and mergeStores.
>
> Instead of just a check of each inst that is a load, check if they are
> mayThrow, and if so, stop, because you can't merge any loads (or sink any
> stores) from that block.
>
I was aware of this before I wrote any of the code :) You'd need the check
in both mergeStores and canSinkFromBlock.
I'd rather just find time to rewrite the whole thing. I think, it's more
like (N^2)*M because mergeStores calls canSinkFromBlock which then does the
canInstructionRangeModRef.
>
>
> ---------- Forwarded message ----------
> From: David Majnemer via llvm-commits <llvm-commits at lists.llvm.org>
> Date: Thu, May 26, 2016 at 12:11 AM
> Subject: [llvm] r270828 - [MergedLoadStoreMotion] Don't transform across
> may-throw calls
> To: llvm-commits at lists.llvm.org
>
>
> Author: majnemer
> Date: Thu May 26 02:11:09 2016
> New Revision: 270828
>
> URL: http://llvm.org/viewvc/llvm-project?rev=270828&view=rev
> Log:
> [MergedLoadStoreMotion] Don't transform across may-throw calls
>
> It is unsafe to hoist a load before a function call which may throw, the
> throw might prevent a pointer dereference.
>
> Likewise, it is unsafe to sink a store after a call which may throw.
> The caller might be able to observe the difference.
>
> This fixes PR27858.
>
> Added:
> llvm/trunk/test/Transforms/InstMerge/exceptions.ll
> Modified:
> llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
> llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll
>
> Modified: llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp?rev=270828&r1=270827&r2=270828&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp Thu May 26
> 02:11:09 2016
> @@ -79,7 +79,6 @@
> #include "llvm/Analysis/Loads.h"
> #include "llvm/Analysis/MemoryBuiltins.h"
> #include "llvm/Analysis/MemoryDependenceAnalysis.h"
> -#include "llvm/Analysis/TargetLibraryInfo.h"
> #include "llvm/IR/Metadata.h"
> #include "llvm/IR/PatternMatch.h"
> #include "llvm/Support/Debug.h"
> @@ -114,7 +113,6 @@ private:
> // This transformation requires dominator postdominator info
> void getAnalysisUsage(AnalysisUsage &AU) const override {
> AU.setPreservesCFG();
> - AU.addRequired<TargetLibraryInfoWrapperPass>();
> AU.addRequired<AAResultsWrapperPass>();
> AU.addPreserved<GlobalsAAWrapperPass>();
> AU.addPreserved<MemoryDependenceWrapperPass>();
> @@ -130,9 +128,9 @@ private:
> BasicBlock *getDiamondTail(BasicBlock *BB);
> bool isDiamondHead(BasicBlock *BB);
> // Routines for hoisting loads
> - bool isLoadHoistBarrierInRange(const Instruction& Start,
> - const Instruction& End,
> - LoadInst* LI);
> + bool isLoadHoistBarrierInRange(const Instruction &Start,
> + const Instruction &End, LoadInst *LI,
> + bool SafeToLoadUnconditionally);
> LoadInst *canHoistFromBlock(BasicBlock *BB, LoadInst *LI);
> void hoistInstruction(BasicBlock *BB, Instruction *HoistCand,
> Instruction *ElseInst);
> @@ -166,7 +164,6 @@ FunctionPass *llvm::createMergedLoadStor
> INITIALIZE_PASS_BEGIN(MergedLoadStoreMotion, "mldst-motion",
> "MergedLoadStoreMotion", false, false)
> INITIALIZE_PASS_DEPENDENCY(MemoryDependenceWrapperPass)
> -INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
> INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass)
> INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)
> INITIALIZE_PASS_END(MergedLoadStoreMotion, "mldst-motion",
> @@ -229,9 +226,14 @@ bool MergedLoadStoreMotion::isDiamondHea
> /// 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 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;
> MemoryLocation Loc = MemoryLocation::get(LI);
> return AA->canInstructionRangeModRef(Start, End, Loc, MRI_Mod);
> }
> @@ -245,23 +247,28 @@ bool MergedLoadStoreMotion::isLoadHoistB
> ///
> LoadInst *MergedLoadStoreMotion::canHoistFromBlock(BasicBlock *BB1,
> LoadInst *Load0) {
> -
> + BasicBlock *BB0 = Load0->getParent();
> + BasicBlock *Head = BB0->getSinglePredecessor();
> + bool SafeToLoadUnconditionally = isSafeToLoadUnconditionally(
> + Load0->getPointerOperand(), Load0->getAlignment(),
> + Load0->getModule()->getDataLayout(),
> + /*ScanFrom=*/Head->getTerminator());
> for (BasicBlock::iterator BBI = BB1->begin(), BBE = BB1->end(); BBI !=
> BBE;
> ++BBI) {
> Instruction *Inst = &*BBI;
>
> // Only merge and hoist loads when their result in used only in BB
> - if (!isa<LoadInst>(Inst) || Inst->isUsedOutsideOfBlock(BB1))
> + auto *Load1 = dyn_cast<LoadInst>(Inst);
> + if (!Load1 || Inst->isUsedOutsideOfBlock(BB1))
> continue;
>
> - auto *Load1 = cast<LoadInst>(Inst);
> - BasicBlock *BB0 = Load0->getParent();
> -
> MemoryLocation Loc0 = MemoryLocation::get(Load0);
> MemoryLocation Loc1 = MemoryLocation::get(Load1);
> if (AA->isMustAlias(Loc0, Loc1) && Load0->isSameOperationAs(Load1) &&
> - !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1) &&
> - !isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0)) {
> + !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1,
> + SafeToLoadUnconditionally) &&
> + !isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0,
> + SafeToLoadUnconditionally)) {
> return Load1;
> }
> }
> @@ -387,6 +394,10 @@ bool MergedLoadStoreMotion::mergeLoads(B
> 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);
> }
>
> @@ -403,18 +414,15 @@ StoreInst *MergedLoadStoreMotion::canSin
> RBI != RBE; ++RBI) {
> Instruction *Inst = &*RBI;
>
> - if (!isa<StoreInst>(Inst))
> - continue;
> -
> - StoreInst *Store1 = cast<StoreInst>(Inst);
> + auto *Store1 = dyn_cast<StoreInst>(Inst);
> + if (!Store1)
> + continue;
>
> MemoryLocation Loc0 = MemoryLocation::get(Store0);
> MemoryLocation Loc1 = MemoryLocation::get(Store1);
> if (AA->isMustAlias(Loc0, Loc1) && Store0->isSameOperationAs(Store1)
> &&
> -
> !isStoreSinkBarrierInRange(*(std::next(BasicBlock::iterator(Store1))),
> - BB1->back(), Loc1) &&
> -
> !isStoreSinkBarrierInRange(*(std::next(BasicBlock::iterator(Store0))),
> - BB0->back(), Loc0)) {
> + !isStoreSinkBarrierInRange(*Store1->getNextNode(), BB1->back(),
> Loc1) &&
> + !isStoreSinkBarrierInRange(*Store0->getNextNode(), BB0->back(),
> Loc0)) {
> return Store1;
> }
> }
>
> Added: llvm/trunk/test/Transforms/InstMerge/exceptions.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstMerge/exceptions.ll?rev=270828&view=auto
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstMerge/exceptions.ll (added)
> +++ llvm/trunk/test/Transforms/InstMerge/exceptions.ll Thu May 26 02:11:09
> 2016
> @@ -0,0 +1,57 @@
> +; RUN: opt -basicaa -memdep -mldst-motion -S < %s | FileCheck %s
> +target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
> +target triple = "x86_64-unknown-linux-gnu"
> +
> + at r = common global i32 0, align 4
> + at s = common global i32 0, align 4
> +
> +; CHECK-LABEL: define void @test1(
> +define void @test1(i1 %cmp, i32* noalias %p) {
> +entry:
> + br i1 %cmp, label %if.then, label %if.else
> +
> +if.then: ; preds = %entry
> + call void @may_throw()
> + %arrayidx = getelementptr inbounds i32, i32* %p, i64 1
> + %0 = load i32, i32* %arrayidx, align 4
> + store i32 %0, i32* @r, align 4
> + br label %if.end
> +; CHECK: call void @may_throw()
> +; CHECK-NEXT: %[[gep:.*]] = getelementptr inbounds i32, i32* %p, i64 1
> +; CHECK-NEXT: %[[load:.*]] = load i32, i32* %[[gep]], align 4
> +; CHECK-NEXT: store i32 %[[load]], i32* @r, align 4
> +
> +if.else: ; preds = %entry
> + %arrayidx1 = getelementptr inbounds i32, i32* %p, i64 1
> + %1 = load i32, i32* %arrayidx1, align 4
> + store i32 %1, i32* @s, align 4
> + br label %if.end
> +
> +if.end: ; preds = %if.else,
> %if.then
> + ret void
> +}
> +
> +; CHECK-LABEL: define void @test2(
> +define void @test2(i1 %cmp, i32* noalias %p) {
> +entry:
> + br i1 %cmp, label %if.then, label %if.else
> +
> +if.then: ; preds = %entry
> + %arrayidx = getelementptr inbounds i32, i32* %p, i64 1
> + store i32 1, i32* %arrayidx, align 4
> + call void @may_throw()
> +; CHECK: %[[gep:.*]] = getelementptr inbounds i32, i32* %p, i64 1
> +; CHECK-NEXT: store i32 1, i32* %[[gep]], align 4
> +; CHECK-NEXT: call void @may_throw()
> + br label %if.end
> +
> +if.else: ; preds = %entry
> + %arrayidx1 = getelementptr inbounds i32, i32* %p, i64 1
> + store i32 2, i32* %arrayidx1, align 4
> + br label %if.end
> +
> +if.end: ; preds = %if.else,
> %if.then
> + ret void
> +}
> +
> +declare void @may_throw()
>
> Modified: llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll?rev=270828&r1=270827&r2=270828&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll
> (original)
> +++ llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll Thu
> May 26 02:11:09 2016
> @@ -33,7 +33,7 @@ if.else:
> %p3 = getelementptr inbounds %struct.node, %struct.node* %node.017, i32
> 0, i32 6
> ; CHECK-NOT: store i32
> store i32 %add, i32* %p3, align 4
> - call i32 @foo(i32 5) ;not a barrier
> + call i32 @foo(i32 5) nounwind ;not a
> barrier
> br label %if.end
>
> ; CHECK: if.end
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160526/298458fa/attachment.html>
More information about the llvm-commits
mailing list