<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 24, 2015 at 4:16 PM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">David,<br>
<br>
I think this is a unnecessary revert. I've responded on the bug with an explanation on why.<br>
<br>
<a href="https://llvm.org/bugs/show_bug.cgi?id=22708" target="_blank">https://llvm.org/bugs/show_<u></u>bug.cgi?id=22708</a></blockquote><div><br></div><div>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.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span class=""><font color="#888888"><br>
<br>
Philip</font></span><div class=""><div class="h5"><br>
<br>
On 03/20/2015 11:19 PM, David Majnemer wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Author: majnemer<br>
Date: Sat Mar 21 01:19:17 2015<br>
New Revision: 232889<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=232889&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=232889&view=rev</a><br>
Log:<br>
MemoryDependenceAnalysis: Don't miscompile atomics<br>
<br>
r216771 introduced a change to MemoryDependenceAnalysis that allowed it<br>
to reason about acquire/release operations. However, this change does<br>
not ensure that the acquire/release operations pair. Unfortunately,<br>
this leads to miscompiles as we won't see an acquire load as properly<br>
memory effecting. This largely reverts r216771.<br>
<br>
This fixes PR22708.<br>
<br>
Modified:<br>
llvm/trunk/lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp<br>
llvm/trunk/test/Transforms/<u></u>DeadStoreElimination/atomic.ll<br>
llvm/trunk/test/Transforms/<u></u>GVN/atomic.ll<br>
<br>
Modified: llvm/trunk/lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=232889&r1=232888&r2=232889&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/lib/<u></u>Analysis/<u></u>MemoryDependenceAnalysis.cpp?<u></u>rev=232889&r1=232888&r2=<u></u>232889&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/<u></u>MemoryDependenceAnalysis.cpp Sat Mar 21 01:19:17 2015<br>
@@ -407,7 +407,6 @@ getPointerDependencyFrom(const AliasAnal<br>
// by every program that can detect any optimisation of that kind: either<br>
// it is racy (undefined) or there is a release followed by an acquire<br>
// between the pair of accesses under consideration.<br>
- bool HasSeenAcquire = false;<br>
if (isLoad && QueryInst) {<br>
LoadInst *LI = dyn_cast<LoadInst>(QueryInst);<br>
@@ -468,12 +467,12 @@ getPointerDependencyFrom(const AliasAnal<br>
// Atomic loads have complications involved.<br>
// A Monotonic (or higher) load is OK if the query inst is itself not atomic.<br>
- // An Acquire (or higher) load sets the HasSeenAcquire flag, so that any<br>
- // release store will know to return getClobber.<br>
// FIXME: This is overly conservative.<br>
if (LI->isAtomic() && LI->getOrdering() > Unordered) {<br>
if (!QueryInst)<br>
return MemDepResult::getClobber(LI);<br>
+ if (LI->getOrdering() != Monotonic)<br>
+ return MemDepResult::getClobber(LI);<br>
if (auto *QueryLI = dyn_cast<LoadInst>(QueryInst)) {<br>
if (!QueryLI->isSimple())<br>
return MemDepResult::getClobber(LI);<br>
@@ -483,9 +482,6 @@ getPointerDependencyFrom(const AliasAnal<br>
} else if (QueryInst-><u></u>mayReadOrWriteMemory()) {<br>
return MemDepResult::getClobber(LI);<br>
}<br>
-<br>
- if (isAtLeastAcquire(LI-><u></u>getOrdering()))<br>
- HasSeenAcquire = true;<br>
}<br>
AliasAnalysis::Location LoadLoc = AA->getLocation(LI);<br>
@@ -545,12 +541,12 @@ getPointerDependencyFrom(const AliasAnal<br>
if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {<br>
// Atomic stores have complications involved.<br>
// A Monotonic store is OK if the query inst is itself not atomic.<br>
- // A Release (or higher) store further requires that no acquire load<br>
- // has been seen.<br>
// FIXME: This is overly conservative.<br>
if (!SI->isUnordered()) {<br>
if (!QueryInst)<br>
return MemDepResult::getClobber(SI);<br>
+ if (SI->getOrdering() != Monotonic)<br>
+ return MemDepResult::getClobber(SI);<br>
if (auto *QueryLI = dyn_cast<LoadInst>(QueryInst)) {<br>
if (!QueryLI->isSimple())<br>
return MemDepResult::getClobber(SI);<br>
@@ -560,9 +556,6 @@ getPointerDependencyFrom(const AliasAnal<br>
} else if (QueryInst-><u></u>mayReadOrWriteMemory()) {<br>
return MemDepResult::getClobber(SI);<br>
}<br>
-<br>
- if (HasSeenAcquire && isAtLeastRelease(SI-><u></u>getOrdering()))<br>
- return MemDepResult::getClobber(SI);<br>
}<br>
// FIXME: this is overly conservative.<br>
<br>
Modified: llvm/trunk/test/Transforms/<u></u>DeadStoreElimination/atomic.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/test/<u></u>Transforms/<u></u>DeadStoreElimination/atomic.<u></u>ll?rev=232889&r1=232888&r2=<u></u>232889&view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/test/Transforms/<u></u>DeadStoreElimination/atomic.ll (original)<br>
+++ llvm/trunk/test/Transforms/<u></u>DeadStoreElimination/atomic.ll Sat Mar 21 01:19:17 2015<br>
@@ -23,28 +23,6 @@ define void @test1() {<br>
ret void<br>
}<br>
-; DSE across seq_cst load (allowed)<br>
-define i32 @test2() {<br>
-; CHECK-LABEL: test2<br>
-; CHECK-NOT: store i32 0<br>
-; CHECK: store i32 1<br>
- store i32 0, i32* @x<br>
- %x = load atomic i32, i32* @y seq_cst, align 4<br>
- store i32 1, i32* @x<br>
- ret i32 %x<br>
-}<br>
-<br>
-; DSE across seq_cst store (allowed)<br>
-define void @test3() {<br>
-; CHECK-LABEL: test3<br>
-; CHECK-NOT: store i32 0<br>
-; CHECK: store atomic i32 2<br>
- store i32 0, i32* @x<br>
- store atomic i32 2, i32* @y seq_cst, align 4<br>
- store i32 1, i32* @x<br>
- ret void<br>
-}<br>
-<br>
; DSE remove unordered store (allowed)<br>
define void @test4() {<br>
; CHECK-LABEL: test4<br>
@@ -141,30 +119,6 @@ define void @test12() {<br>
ret void<br>
}<br>
-; DSE is allowed across a pair of an atomic read and then write.<br>
-define i32 @test13() {<br>
-; CHECK-LABEL: test13<br>
-; CHECK-NOT: store i32 0<br>
-; CHECK: store i32 1<br>
- store i32 0, i32* @x<br>
- %x = load atomic i32, i32* @y seq_cst, align 4<br>
- store atomic i32 %x, i32* @y seq_cst, align 4<br>
- store i32 1, i32* @x<br>
- ret i32 %x<br>
-}<br>
-<br>
-; Same if it is acquire-release instead of seq_cst/seq_cst<br>
-define i32 @test14() {<br>
-; CHECK-LABEL: test14<br>
-; CHECK-NOT: store i32 0<br>
-; CHECK: store i32 1<br>
- store i32 0, i32* @x<br>
- %x = load atomic i32, i32* @y acquire, align 4<br>
- store atomic i32 %x, i32* @y release, align 4<br>
- store i32 1, i32* @x<br>
- ret i32 %x<br>
-}<br>
-<br>
; But DSE is not allowed across a release-acquire pair.<br>
define i32 @test15() {<br>
; CHECK-LABEL: test15<br>
<br>
Modified: llvm/trunk/test/Transforms/<u></u>GVN/atomic.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project/llvm/trunk/test/<u></u>Transforms/GVN/atomic.ll?rev=<u></u>232889&r1=232888&r2=232889&<u></u>view=diff</a><br>
==============================<u></u>==============================<u></u>==================<br>
--- llvm/trunk/test/Transforms/<u></u>GVN/atomic.ll (original)<br>
+++ llvm/trunk/test/Transforms/<u></u>GVN/atomic.ll Sat Mar 21 01:19:17 2015<br>
@@ -18,18 +18,6 @@ entry:<br>
ret i32 %z<br>
}<br>
-; GVN across seq_cst store (allowed)<br>
-define i32 @test2() nounwind uwtable ssp {<br>
-; CHECK-LABEL: test2<br>
-; CHECK: add i32 %x, %x<br>
-entry:<br>
- %x = load i32, i32* @y<br>
- store atomic i32 %x, i32* @x seq_cst, align 4<br>
- %y = load i32, i32* @y<br>
- %z = add i32 %x, %y<br>
- ret i32 %z<br>
-}<br>
-<br>
; GVN across unordered load (allowed)<br>
define i32 @test3() nounwind uwtable ssp {<br>
; CHECK-LABEL: test3<br>
@@ -43,20 +31,6 @@ entry:<br>
ret i32 %b<br>
}<br>
-; GVN across acquire load (allowed as the original load was not atomic)<br>
-define i32 @test4() nounwind uwtable ssp {<br>
-; CHECK-LABEL: test4<br>
-; CHECK: load atomic i32, i32* @x<br>
-; CHECK-NOT: load i32, i32* @y<br>
-entry:<br>
- %x = load i32, i32* @y<br>
- %y = load atomic i32, i32* @x seq_cst, align 4<br>
- %x2 = load i32, i32* @y<br>
- %x3 = add i32 %x, %x2<br>
- %y2 = add i32 %y, %x3<br>
- ret i32 %y2<br>
-}<br>
-<br>
; GVN load to unordered load (allowed)<br>
define i32 @test5() nounwind uwtable ssp {<br>
; CHECK-LABEL: test5<br>
@@ -92,19 +66,6 @@ entry:<br>
ret i32 %z<br>
}<br>
-; GVN across acquire-release pair (allowed)<br>
-define i32 @test8() nounwind uwtable ssp {<br>
-; CHECK-LABEL: test8<br>
-; CHECK: add i32 %x, %x<br>
-entry:<br>
- %x = load i32, i32* @y<br>
- %w = load atomic i32, i32* @x acquire, align 4<br>
- store atomic i32 %x, i32* @x release, align 4<br>
- %y = load i32, i32* @y<br>
- %z = add i32 %x, %y<br>
- ret i32 %z<br>
-}<br>
-<br>
; GVN across monotonic store (allowed)<br>
define i32 @test9() nounwind uwtable ssp {<br>
; CHECK-LABEL: test9<br>
@@ -129,3 +90,20 @@ entry:<br>
ret i32 %z<br>
}<br>
+define i32 @PR22708(i1 %flag) {<br>
+; CHECK-LABEL: PR22708<br>
+entry:<br>
+ br i1 %flag, label %if.then, label %if.end<br>
+<br>
+if.then:<br>
+ store i32 43, i32* @y, align 4<br>
+; CHECK: store i32 43, i32* @y, align 4<br>
+ br label %if.end<br>
+<br>
+if.end:<br>
+ load atomic i32, i32* @x acquire, align 4<br>
+ %load = load i32, i32* @y, align 4<br>
+; CHECK: load atomic i32, i32* @x acquire, align 4<br>
+; CHECK: load i32, i32* @y, align 4<br>
+ ret i32 %load<br>
+}<br>
<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
</div></div></blockquote></div><br></div></div>