[llvm] r232889 - MemoryDependenceAnalysis: Don't miscompile atomics
David Majnemer
david.majnemer at gmail.com
Tue Mar 24 23:13:43 PDT 2015
On Tue, Mar 24, 2015 at 4:16 PM, Philip Reames <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.
>
>
> 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
>> 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/20150324/0a6c4b5a/attachment.html>
More information about the llvm-commits
mailing list