[llvm] r283965 - GVN-hoist: fix store past load dependence analysis (PR30216, PR30499)
Reid Kleckner via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 12 16:48:24 PDT 2016
I suspect this causes a crash on valid during the Chromium build:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxLLD/builds/1719/steps/compile/logs/stdio
The crash is during GVN hoisting:
3. Running pass 'Early GVN Hoisting of Expressions' on function
'@hb_buffer_deserialize_glyphs'
Your new helper is also on the stack:
#8 0x0000000001d0a6af llvm::defClobbersUseOrDef(llvm::MemoryDef*,
llvm::MemoryUseOrDef const*, llvm::AAResults&)
I will try to get a reproducer.
On Tue, Oct 11, 2016 at 7:23 PM, Sebastian Pop via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: spop
> Date: Tue Oct 11 21:23:39 2016
> New Revision: 283965
>
> URL: http://llvm.org/viewvc/llvm-project?rev=283965&view=rev
> Log:
> GVN-hoist: fix store past load dependence analysis (PR30216, PR30499)
>
> This is a refreshed version of a patch that was reverted: it fixes
> the problems reported in both PR30216 and PR30499, and
> contains all the test-cases from both bugs.
>
> 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.
>
> Tested on x86_64-linux with check and a test-suite run.
>
> Differential Revision: https://reviews.llvm.org/D25476
>
> Added:
> llvm/trunk/test/Transforms/GVNHoist/pr30216.ll
> llvm/trunk/test/Transforms/GVNHoist/pr30499.ll
> Modified:
> llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h
> llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
> llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
>
> Modified: llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
> llvm/Transforms/Utils/MemorySSA.h?rev=283965&r1=283964&r2=283965&view=diff
> ============================================================
> ==================
> --- llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h (original)
> +++ llvm/trunk/include/llvm/Transforms/Utils/MemorySSA.h Tue Oct 11
> 21:23:39 2016
> @@ -974,6 +974,10 @@ inline upward_defs_iterator upward_defs_
>
> inline upward_defs_iterator upward_defs_end() { return
> upward_defs_iterator(); }
>
> +bool instructionClobbersQuery(MemoryDef *MD,
> + const MemoryLocation &UseLoc,
> + const Instruction *UseInst,
> + AliasAnalysis &AA);
> } // end namespace llvm
>
> #endif // LLVM_TRANSFORMS_UTILS_MEMORYSSA_H
>
> Modified: llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/Scalar/GVNHoist.cpp?rev=283965&r1=283964&r2=283965&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp Tue Oct 11 21:23:39 2016
> @@ -329,31 +329,36 @@ 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 hasMemoryUse(const Instruction *NewPt, MemoryDef *Def,
> + const BasicBlock *BB) {
> + const MemorySSA::AccessList *Acc = MSSA->getBlockAccesses(BB);
> + if (!Acc)
> + return false;
> +
> + Instruction *OldPt = Def->getMemoryInst();
> const BasicBlock *OldBB = OldPt->getParent();
> + const BasicBlock *NewBB = NewPt->getParent();
> + bool ReachedNewPt = 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)
> + if (const MemoryUse *MU = dyn_cast<MemoryUse>(&MA)) {
> + Instruction *Insn = MU->getMemoryInst();
> +
> + // Do not check whether MU aliases Def when MU occurs after OldPt.
> + if (BB == OldBB && firstInBB(OldPt, Insn))
> + break;
> +
> + // Do not check whether MU aliases Def when MU occurs before
> NewPt.
> + if (BB == NewBB) {
> + if (!ReachedNewPt) {
> + if (firstInBB(Insn, NewPt))
> + continue;
> + ReachedNewPt = true;
> + }
> }
> -
> - if (UBB != OldBB)
> - return true;
> -
> - // It is only harmful to hoist when the use is before OldPt.
> - if (firstInBB(MU->getMemoryInst(), OldPt))
> + if (instructionClobbersQuery(Def, MemoryLocation::get(Insn), Insn,
> + *AA))
> return true;
> }
>
> @@ -361,17 +366,18 @@ private:
> }
>
> // 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 +396,7 @@ private:
> return true;
>
> // Check that we do not move a store past loads.
> - if (hasMemoryUseOnPath(Def, *I, OldPt))
> + if (hasMemoryUse(NewPt, Def, *I))
> return true;
>
> // Stop walk once the limit is reached.
> @@ -473,7 +479,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<MemoryDef>(U),
> NBBsOnAllPaths))
> return false;
> } else if (hasEHOnPath(NewBB, OldBB, NBBsOnAllPaths))
> return false;
>
> Modified: llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/Utils/MemorySSA.cpp?rev=283965&r1=283964&r2=283965&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/MemorySSA.cpp Tue Oct 11 21:23:39 2016
> @@ -169,44 +169,6 @@ template <> struct DenseMapInfo<MemoryLo
> return LHS == RHS;
> }
> };
> -}
> -
> -namespace {
> -struct UpwardsMemoryQuery {
> - // True if our original query started off as a call
> - bool IsCall;
> - // The pointer location we started the query with. This will be empty if
> - // IsCall is true.
> - MemoryLocation StartingLoc;
> - // This is the instruction we were querying about.
> - const Instruction *Inst;
> - // The MemoryAccess we actually got called with, used to test local
> domination
> - const MemoryAccess *OriginalAccess;
> -
> - UpwardsMemoryQuery()
> - : IsCall(false), Inst(nullptr), OriginalAccess(nullptr) {}
> -
> - UpwardsMemoryQuery(const Instruction *Inst, const MemoryAccess *Access)
> - : IsCall(ImmutableCallSite(Inst)), Inst(Inst),
> OriginalAccess(Access) {
> - if (!IsCall)
> - StartingLoc = MemoryLocation::get(Inst);
> - }
> -};
> -
> -static bool lifetimeEndsAt(MemoryDef *MD, const MemoryLocation &Loc,
> - AliasAnalysis &AA) {
> - Instruction *Inst = MD->getMemoryInst();
> - if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
> - switch (II->getIntrinsicID()) {
> - case Intrinsic::lifetime_start:
> - case Intrinsic::lifetime_end:
> - return AA.isMustAlias(MemoryLocation(II->getArgOperand(1)), Loc);
> - default:
> - return false;
> - }
> - }
> - return false;
> -}
>
> enum class Reorderability { Always, IfNoAlias, Never };
>
> @@ -248,21 +210,10 @@ static Reorderability getLoadReorderabil
> return Result;
> }
>
> -static bool isUseTriviallyOptimizableToLiveOnEntry(AliasAnalysis &AA,
> - const Instruction *I) {
> - // If the memory can't be changed, then loads of the memory can't be
> - // clobbered.
> - //
> - // FIXME: We should handle invariant groups, as well. It's a bit harder,
> - // because we need to pay close attention to invariant group barriers.
> - return isa<LoadInst>(I) && (I->getMetadata(LLVMContext::MD_invariant_load)
> ||
> - AA.pointsToConstantMemory(I));
> -}
> -
> -static bool instructionClobbersQuery(MemoryDef *MD,
> - const MemoryLocation &UseLoc,
> - const Instruction *UseInst,
> - AliasAnalysis &AA) {
> +bool instructionClobbersQuery(MemoryDef *MD,
> + const MemoryLocation &UseLoc,
> + const Instruction *UseInst,
> + AliasAnalysis &AA) {
> Instruction *DefInst = MD->getMemoryInst();
> assert(DefInst && "Defining instruction not actually an instruction");
>
> @@ -303,6 +254,56 @@ static bool instructionClobbersQuery(Mem
> return AA.getModRefInfo(DefInst, UseLoc) & MRI_Mod;
> }
>
> +}
> +
> +namespace {
> +struct UpwardsMemoryQuery {
> + // True if our original query started off as a call
> + bool IsCall;
> + // The pointer location we started the query with. This will be empty if
> + // IsCall is true.
> + MemoryLocation StartingLoc;
> + // This is the instruction we were querying about.
> + const Instruction *Inst;
> + // The MemoryAccess we actually got called with, used to test local
> domination
> + const MemoryAccess *OriginalAccess;
> +
> + UpwardsMemoryQuery()
> + : IsCall(false), Inst(nullptr), OriginalAccess(nullptr) {}
> +
> + UpwardsMemoryQuery(const Instruction *Inst, const MemoryAccess *Access)
> + : IsCall(ImmutableCallSite(Inst)), Inst(Inst),
> OriginalAccess(Access) {
> + if (!IsCall)
> + StartingLoc = MemoryLocation::get(Inst);
> + }
> +};
> +
> +static bool lifetimeEndsAt(MemoryDef *MD, const MemoryLocation &Loc,
> + AliasAnalysis &AA) {
> + Instruction *Inst = MD->getMemoryInst();
> + if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst)) {
> + switch (II->getIntrinsicID()) {
> + case Intrinsic::lifetime_start:
> + case Intrinsic::lifetime_end:
> + return AA.isMustAlias(MemoryLocation(II->getArgOperand(1)), Loc);
> + default:
> + return false;
> + }
> + }
> + return false;
> +}
> +
> +static bool isUseTriviallyOptimizableToLiveOnEntry(AliasAnalysis &AA,
> + const Instruction *I) {
> + // If the memory can't be changed, then loads of the memory can't be
> + // clobbered.
> + //
> + // FIXME: We should handle invariant groups, as well. It's a bit harder,
> + // because we need to pay close attention to invariant group barriers.
> + return isa<LoadInst>(I) && (I->getMetadata(LLVMContext::MD_invariant_load)
> ||
> + AA.pointsToConstantMemory(I));
> +}
> +
> static bool instructionClobbersQuery(MemoryDef *MD, MemoryUse *MU,
> const MemoryLocOrCall &UseMLOC,
> AliasAnalysis &AA) {
>
> Added: llvm/trunk/test/Transforms/GVNHoist/pr30216.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/GVNHoist/pr30216.ll?rev=283965&view=auto
> ============================================================
> ==================
> --- llvm/trunk/test/Transforms/GVNHoist/pr30216.ll (added)
> +++ llvm/trunk/test/Transforms/GVNHoist/pr30216.ll Tue Oct 11 21:23:39
> 2016
> @@ -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
> +
> + 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
> +}
>
> Added: llvm/trunk/test/Transforms/GVNHoist/pr30499.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/GVNHoist/pr30499.ll?rev=283965&view=auto
> ============================================================
> ==================
> --- llvm/trunk/test/Transforms/GVNHoist/pr30499.ll (added)
> +++ llvm/trunk/test/Transforms/GVNHoist/pr30499.ll Tue Oct 11 21:23:39
> 2016
> @@ -0,0 +1,30 @@
> +; RUN: opt -S -gvn-hoist < %s
> +
> +define void @_Z3fn2v() #0 {
> +entry:
> + %a = alloca i8*, align 8
> + %b = alloca i32, align 4
> + %0 = load i8*, i8** %a, align 8
> + store i8 0, i8* %0, align 1
> + %1 = load i32, i32* %b, align 4
> + %tobool = icmp ne i32 %1, 0
> + br i1 %tobool, label %if.then, label %if.end
> +
> +if.then: ; preds = %entry
> + %call = call i64 @_Z3fn1v() #2
> + %conv = trunc i64 %call to i32
> + store i32 %conv, i32* %b, align 4
> + br label %if.end
> +
> +if.end: ; preds = %if.then,
> %entry
> + %2 = load i8*, i8** %a, align 8
> + store i8 0, i8* %2, align 1
> + ret void
> +}
> +
> +; Function Attrs: nounwind readonly
> +declare i64 @_Z3fn1v() #1
> +
> +attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false"
> "disable-tail-calls"="false" "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="false" "no-infs-fp-math"="false"
> "no-jump-tables"="false" "no-nans-fp-math"="false"
> "no-signed-zeros-fp-math"="false" "no-trapping-math"="false"
> "stack-protector-buffer-size"="8" "target-cpu"="x86-64"
> "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false"
> "use-soft-float"="false" }
> +attributes #1 = { nounwind readonly "correctly-rounded-divide-sqrt-fp-math"="false"
> "disable-tail-calls"="false" "less-precise-fpmad"="false"
> "no-frame-pointer-elim"="false" "no-infs-fp-math"="false"
> "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false"
> "no-trapping-math"="false" "stack-protector-buffer-size"="8"
> "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87"
> "unsafe-fp-math"="false" "use-soft-float"="false" }
> +attributes #2 = { nounwind readonly }
>
>
> _______________________________________________
> 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/20161012/85689266/attachment.html>
More information about the llvm-commits
mailing list