[llvm] r232889 - MemoryDependenceAnalysis: Don't miscompile atomics
Philip Reames
listmail at philipreames.com
Thu Apr 9 08:47:22 PDT 2015
On 03/24/2015 11:13 PM, David Majnemer wrote:
>
>
> On Tue, Mar 24, 2015 at 4:16 PM, Philip Reames
> <listmail at philipreames.com <mailto:listmail at philipreames.com>> wrote:
>
> David,
>
> I think this is a unnecessary revert. I've responded on the bug
> with an explanation on why.
>
> https://llvm.org/bugs/show_bug.cgi?id=22708
>
>
> The revert was necessary in so much as it fixes a miscompile. I
> estimated that the amount of code it would take to repair the
> optimization was more than the delta introduced by reverting r216771.
> The fix I considered involved trying to hold on to the load while we
> continued scanning for the release-store. If we saw any instruction
> which seemed like a clobber while scanning, we should instead return
> the load. If there is a better or otherwise more concise solution,
> I'd be happy to see it in LLVM.
Just to close the loop on this, after discussion on the bug I've
withdrawn my objections. I was reading something out of context. After
adjusting for that, I do not see an obvious small fix to the issue at
hand and the revert is entirely appropriate.
>
>
>
> Philip
>
>
> On 03/20/2015 11:19 PM, David Majnemer wrote:
>
> Author: majnemer
> Date: Sat Mar 21 01:19:17 2015
> New Revision: 232889
>
> URL: http://llvm.org/viewvc/llvm-project?rev=232889&view=rev
> Log:
> MemoryDependenceAnalysis: Don't miscompile atomics
>
> r216771 introduced a change to MemoryDependenceAnalysis that
> allowed it
> to reason about acquire/release operations. However, this
> change does
> not ensure that the acquire/release operations pair.
> Unfortunately,
> this leads to miscompiles as we won't see an acquire load as
> properly
> memory effecting. This largely reverts r216771.
>
> This fixes PR22708.
>
> Modified:
> llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp
> llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll
> llvm/trunk/test/Transforms/GVN/atomic.ll
>
> Modified: llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=232889&r1=232888&r2=232889&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp
> (original)
> +++ llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp Sat
> Mar 21 01:19:17 2015
> @@ -407,7 +407,6 @@ getPointerDependencyFrom(const AliasAnal
> // by every program that can detect any optimisation of
> that kind: either
> // it is racy (undefined) or there is a release followed
> by an acquire
> // between the pair of accesses under consideration.
> - bool HasSeenAcquire = false;
> if (isLoad && QueryInst) {
> LoadInst *LI = dyn_cast<LoadInst>(QueryInst);
> @@ -468,12 +467,12 @@ getPointerDependencyFrom(const AliasAnal
> // Atomic loads have complications involved.
> // A Monotonic (or higher) load is OK if the query
> inst is itself not atomic.
> - // An Acquire (or higher) load sets the HasSeenAcquire
> flag, so that any
> - // release store will know to return getClobber.
> // FIXME: This is overly conservative.
> if (LI->isAtomic() && LI->getOrdering() > Unordered) {
> if (!QueryInst)
> return MemDepResult::getClobber(LI);
> + if (LI->getOrdering() != Monotonic)
> + return MemDepResult::getClobber(LI);
> if (auto *QueryLI = dyn_cast<LoadInst>(QueryInst)) {
> if (!QueryLI->isSimple())
> return MemDepResult::getClobber(LI);
> @@ -483,9 +482,6 @@ getPointerDependencyFrom(const AliasAnal
> } else if (QueryInst->mayReadOrWriteMemory()) {
> return MemDepResult::getClobber(LI);
> }
> -
> - if (isAtLeastAcquire(LI->getOrdering()))
> - HasSeenAcquire = true;
> }
> AliasAnalysis::Location LoadLoc = AA->getLocation(LI);
> @@ -545,12 +541,12 @@ getPointerDependencyFrom(const AliasAnal
> if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
> // Atomic stores have complications involved.
> // A Monotonic store is OK if the query inst is itself
> not atomic.
> - // A Release (or higher) store further requires that no
> acquire load
> - // has been seen.
> // FIXME: This is overly conservative.
> if (!SI->isUnordered()) {
> if (!QueryInst)
> return MemDepResult::getClobber(SI);
> + if (SI->getOrdering() != Monotonic)
> + return MemDepResult::getClobber(SI);
> if (auto *QueryLI = dyn_cast<LoadInst>(QueryInst)) {
> if (!QueryLI->isSimple())
> return MemDepResult::getClobber(SI);
> @@ -560,9 +556,6 @@ getPointerDependencyFrom(const AliasAnal
> } else if (QueryInst->mayReadOrWriteMemory()) {
> return MemDepResult::getClobber(SI);
> }
> -
> - if (HasSeenAcquire &&
> isAtLeastRelease(SI->getOrdering()))
> - return MemDepResult::getClobber(SI);
> }
> // FIXME: this is overly conservative.
>
> Modified:
> llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll
> (original)
> +++ llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll
> Sat Mar 21 01:19:17 2015
> @@ -23,28 +23,6 @@ define void @test1() {
> ret void
> }
> -; DSE across seq_cst load (allowed)
> -define i32 @test2() {
> -; CHECK-LABEL: test2
> -; CHECK-NOT: store i32 0
> -; CHECK: store i32 1
> - store i32 0, i32* @x
> - %x = load atomic i32, i32* @y seq_cst, align 4
> - store i32 1, i32* @x
> - ret i32 %x
> -}
> -
> -; DSE across seq_cst store (allowed)
> -define void @test3() {
> -; CHECK-LABEL: test3
> -; CHECK-NOT: store i32 0
> -; CHECK: store atomic i32 2
> - store i32 0, i32* @x
> - store atomic i32 2, i32* @y seq_cst, align 4
> - store i32 1, i32* @x
> - ret void
> -}
> -
> ; DSE remove unordered store (allowed)
> define void @test4() {
> ; CHECK-LABEL: test4
> @@ -141,30 +119,6 @@ define void @test12() {
> ret void
> }
> -; DSE is allowed across a pair of an atomic read and then
> write.
> -define i32 @test13() {
> -; CHECK-LABEL: test13
> -; CHECK-NOT: store i32 0
> -; CHECK: store i32 1
> - store i32 0, i32* @x
> - %x = load atomic i32, i32* @y seq_cst, align 4
> - store atomic i32 %x, i32* @y seq_cst, align 4
> - store i32 1, i32* @x
> - ret i32 %x
> -}
> -
> -; Same if it is acquire-release instead of seq_cst/seq_cst
> -define i32 @test14() {
> -; CHECK-LABEL: test14
> -; CHECK-NOT: store i32 0
> -; CHECK: store i32 1
> - store i32 0, i32* @x
> - %x = load atomic i32, i32* @y acquire, align 4
> - store atomic i32 %x, i32* @y release, align 4
> - store i32 1, i32* @x
> - ret i32 %x
> -}
> -
> ; But DSE is not allowed across a release-acquire pair.
> define i32 @test15() {
> ; CHECK-LABEL: test15
>
> Modified: llvm/trunk/test/Transforms/GVN/atomic.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/GVN/atomic.ll (original)
> +++ llvm/trunk/test/Transforms/GVN/atomic.ll Sat Mar 21
> 01:19:17 2015
> @@ -18,18 +18,6 @@ entry:
> ret i32 %z
> }
> -; GVN across seq_cst store (allowed)
> -define i32 @test2() nounwind uwtable ssp {
> -; CHECK-LABEL: test2
> -; CHECK: add i32 %x, %x
> -entry:
> - %x = load i32, i32* @y
> - store atomic i32 %x, i32* @x seq_cst, align 4
> - %y = load i32, i32* @y
> - %z = add i32 %x, %y
> - ret i32 %z
> -}
> -
> ; GVN across unordered load (allowed)
> define i32 @test3() nounwind uwtable ssp {
> ; CHECK-LABEL: test3
> @@ -43,20 +31,6 @@ entry:
> ret i32 %b
> }
> -; GVN across acquire load (allowed as the original load was
> not atomic)
> -define i32 @test4() nounwind uwtable ssp {
> -; CHECK-LABEL: test4
> -; CHECK: load atomic i32, i32* @x
> -; CHECK-NOT: load i32, i32* @y
> -entry:
> - %x = load i32, i32* @y
> - %y = load atomic i32, i32* @x seq_cst, align 4
> - %x2 = load i32, i32* @y
> - %x3 = add i32 %x, %x2
> - %y2 = add i32 %y, %x3
> - ret i32 %y2
> -}
> -
> ; GVN load to unordered load (allowed)
> define i32 @test5() nounwind uwtable ssp {
> ; CHECK-LABEL: test5
> @@ -92,19 +66,6 @@ entry:
> ret i32 %z
> }
> -; GVN across acquire-release pair (allowed)
> -define i32 @test8() nounwind uwtable ssp {
> -; CHECK-LABEL: test8
> -; CHECK: add i32 %x, %x
> -entry:
> - %x = load i32, i32* @y
> - %w = load atomic i32, i32* @x acquire, align 4
> - store atomic i32 %x, i32* @x release, align 4
> - %y = load i32, i32* @y
> - %z = add i32 %x, %y
> - ret i32 %z
> -}
> -
> ; GVN across monotonic store (allowed)
> define i32 @test9() nounwind uwtable ssp {
> ; CHECK-LABEL: test9
> @@ -129,3 +90,20 @@ entry:
> ret i32 %z
> }
> +define i32 @PR22708(i1 %flag) {
> +; CHECK-LABEL: PR22708
> +entry:
> + br i1 %flag, label %if.then, label %if.end
> +
> +if.then:
> + store i32 43, i32* @y, align 4
> +; CHECK: store i32 43, i32* @y, align 4
> + br label %if.end
> +
> +if.end:
> + load atomic i32, i32* @x acquire, align 4
> + %load = load i32, i32* @y, align 4
> +; CHECK: load atomic i32, i32* @x acquire, align 4
> +; CHECK: load i32, i32* @y, align 4
> + ret i32 %load
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150409/dfab5035/attachment.html>
More information about the llvm-commits
mailing list