<div dir="ltr">Sorry about doing two commits instead of one for this revision, it was a mistake on my side (forgot about the behaviour of git svn).</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Aug 18, 2014 at 3:18 PM, Robin Morisset <span dir="ltr"><<a href="mailto:morisset@google.com" target="_blank">morisset@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: morisset<br>
Date: Mon Aug 18 17:18:14 2014<br>
New Revision: 215943<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=215943&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=215943&view=rev</a><br>
Log:<br>
Answer to Philip Reames comments<br>
<br>
- add check for volatile (probably unneeded, but I agree that we should be conservative about it).<br>
- strengthen condition from isUnordered() to isSimple(), as I don't understand well enough Unordered semantics (and it also matches the comment better this way) to be confident in the previous behaviour (thanks for catching that one, I had missed the case Monotonic/Unordered).<br>
- separate a condition in two.<br>
- lengthen comment about aliasing and loads<br>
- add tests in GVN/atomic.ll<br>
<br>
Modified:<br>
llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp<br>
llvm/trunk/test/Transforms/GVN/atomic.ll<br>
<br>
Modified: llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=215943&r1=215942&r2=215943&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp?rev=215943&r1=215942&r2=215943&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/MemoryDependenceAnalysis.cpp Mon Aug 18 17:18:14 2014<br>
@@ -407,21 +407,33 @@ getPointerDependencyFrom(const AliasAnal<br>
<br>
// Values depend on loads if the pointers are must aliased. This means that<br>
// a load depends on another must aliased load from the same value.<br>
+ // One exception is atomic loads: a value can depend on an atomic load that it<br>
+ // does not alias with when this atomic load indicates that another thread may<br>
+ // be accessing the location.<br>
if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {<br>
// Atomic loads have complications involved.<br>
// A monotonic load is OK if the query inst is itself not atomic.<br>
// FIXME: This is overly conservative.<br>
if (!LI->isUnordered()) {<br>
- if (!QueryInst || LI->getOrdering() != Monotonic)<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->isUnordered())<br>
+ if (!QueryLI->isSimple())<br>
return MemDepResult::getClobber(LI);<br>
if (auto *QuerySI = dyn_cast<StoreInst>(QueryInst))<br>
- if (!QuerySI->isUnordered())<br>
+ if (!QuerySI->isSimple())<br>
return MemDepResult::getClobber(LI);<br>
}<br>
<br>
+ // FIXME: this is overly conservative.<br>
+ // While volatile access cannot be eliminated, they do not have to clobber<br>
+ // non-aliasing locations, as normal accesses can for example be reordered<br>
+ // with volatile accesses.<br>
+ if (LI->isVolatile())<br>
+ return MemDepResult::getClobber(LI);<br>
+<br>
AliasAnalysis::Location LoadLoc = AA->getLocation(LI);<br>
<br>
// If we found a pointer, check if it could be the same as our pointer.<br>
@@ -481,16 +493,25 @@ getPointerDependencyFrom(const AliasAnal<br>
// A monotonic store is OK if the query inst is itself not atomic.<br>
// FIXME: This is overly conservative.<br>
if (!SI->isUnordered()) {<br>
- if (!QueryInst || SI->getOrdering() != Monotonic)<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->isUnordered())<br>
+ if (!QueryLI->isSimple())<br>
return MemDepResult::getClobber(SI);<br>
if (auto *QuerySI = dyn_cast<StoreInst>(QueryInst))<br>
- if (!QuerySI->isUnordered())<br>
+ if (!QuerySI->isSimple())<br>
return MemDepResult::getClobber(SI);<br>
}<br>
<br>
+ // FIXME: this is overly conservative.<br>
+ // While volatile access cannot be eliminated, they do not have to clobber<br>
+ // non-aliasing locations, as normal accesses can for example be reordered<br>
+ // with volatile accesses.<br>
+ if (SI->isVolatile())<br>
+ return MemDepResult::getClobber(SI);<br>
+<br>
// If alias analysis can tell that this store is guaranteed to not modify<br>
// the query pointer, ignore it. Use getModRefInfo to handle cases where<br>
// the query pointer points to constant memory etc.<br>
<br>
Modified: llvm/trunk/test/Transforms/GVN/atomic.ll<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/atomic.ll?rev=215943&r1=215942&r2=215943&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/atomic.ll?rev=215943&r1=215942&r2=215943&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/test/Transforms/GVN/atomic.ll (original)<br>
+++ llvm/trunk/test/Transforms/GVN/atomic.ll Mon Aug 18 17:18:14 2014<br>
@@ -78,3 +78,29 @@ entry:<br>
%x3 = add i32 %x, %x2<br>
ret i32 %x3<br>
}<br>
+<br>
+; GVN across monotonic store (allowed)<br>
+define i32 @test7() nounwind uwtable ssp {<br>
+; CHECK: test7<br>
+; CHECK: add i32 %x, %x<br>
+entry:<br>
+ %x = load i32* @y<br>
+ store atomic i32 %x, i32* @x monotonic, align 4<br>
+ %y = load i32* @y<br>
+ %z = add i32 %x, %y<br>
+ ret i32 %z<br>
+}<br>
+<br>
+; GVN of an unordered across monotonic load (not allowed)<br>
+define i32 @test8() nounwind uwtable ssp {<br>
+; CHECK: test8<br>
+; CHECK: add i32 %x, %y<br>
+entry:<br>
+ %x = load atomic i32* @y unordered, align 4<br>
+ %clobber = load atomic i32* @x monotonic, align 4<br>
+ %y = load atomic i32* @y monotonic, align 4<br>
+ %z = add i32 %x, %y<br>
+ ret i32 %z<br>
+}<br>
+<br>
+<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">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/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>