<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 03/24/2015 11:13 PM, David Majnemer
wrote:<br>
</div>
<blockquote
cite="mid:CAL7bZ_cTEpRj5g8VQfYFREH3YYxYKNCJeCypHHSnqsefP3z63Q@mail.gmail.com"
type="cite">
<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 moz-do-not-send="true"
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 moz-do-not-send="true"
href="https://llvm.org/bugs/show_bug.cgi?id=22708"
target="_blank">https://llvm.org/bugs/show_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>
</div>
</div>
</blockquote>
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. <br>
<blockquote
cite="mid:CAL7bZ_cTEpRj5g8VQfYFREH3YYxYKNCJeCypHHSnqsefP3z63Q@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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 moz-do-not-send="true"
href="http://llvm.org/viewvc/llvm-project?rev=232889&view=rev"
target="_blank">http://llvm.org/viewvc/llvm-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/MemoryDependenceAnalysis.cpp<br>
llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll<br>
llvm/trunk/test/Transforms/GVN/atomic.ll<br>
<br>
Modified: llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp<br>
URL: <a moz-do-not-send="true"
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-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=232889&r1=232888&r2=232889&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp
(original)<br>
+++ llvm/trunk/lib/Analysis/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->mayReadOrWriteMemory())
{<br>
return MemDepResult::getClobber(LI);<br>
}<br>
-<br>
- if (isAtLeastAcquire(LI->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->mayReadOrWriteMemory())
{<br>
return MemDepResult::getClobber(SI);<br>
}<br>
-<br>
- if (HasSeenAcquire &&
isAtLeastRelease(SI->getOrdering()))<br>
- return MemDepResult::getClobber(SI);<br>
}<br>
// FIXME: this is overly conservative.<br>
<br>
Modified: llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll<br>
URL: <a moz-do-not-send="true"
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-project/llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/DeadStoreElimination/atomic.ll
(original)<br>
+++ llvm/trunk/test/Transforms/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/GVN/atomic.ll<br>
URL: <a moz-do-not-send="true"
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-project/llvm/trunk/test/Transforms/GVN/atomic.ll?rev=232889&r1=232888&r2=232889&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/GVN/atomic.ll
(original)<br>
+++ llvm/trunk/test/Transforms/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>
_______________________________________________<br>
llvm-commits mailing list<br>
<a moz-do-not-send="true"
href="mailto:llvm-commits@cs.uiuc.edu"
target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a moz-do-not-send="true"
href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits"
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
</div>
</div>
</blockquote>
</div>
<br>
</div>
</div>
</blockquote>
<br>
</body>
</html>