[PATCH] Further relax the constraint on atomics in MemoryDependencyAnalysis.cpp

[Philip Reames]

Comments inline.  I am not yet convinced of the correctness.

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:388
@@ -373,1 +387,3 @@
+  bool HasSeenAcquire = false;
+
   if (isLoad && QueryInst) {
----------------
I'm still not finding this explanation particularly clear.  I'm going to ask you not to commit this until you can justify why this approach is correct.  

A potentially more intuitive way to explain this is to why this reordering is valid:
store x = 0
acquire operator
release operation
load x < -- okay to use zero

This is valid because the first pair of operations and the second pair can both be reordered.  i.e. the intermediate state is:
acquire operation
store x = 0
load x < -- now obviously zero
release operation

What is "discriminating context"?  What does it mean to "clobber"?


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:440
@@ -423,3 +439,3 @@
             return MemDepResult::getClobber(LI);
         if (auto *QuerySI = dyn_cast<StoreInst>(QueryInst))
           if (!QuerySI->isSimple())
----------------
This is a bit off topic for this review, but I may have spotted a bug here.  What if the query instruction is a RMW operation?  These can have ordering semantics, but the existing code (from your previous change) would say there's no dependence.  

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:443
@@ -426,1 +442,3 @@
             return MemDepResult::getClobber(LI);
+        if (isAtLeastAcquire(LI->getOrdering()))
+          HasSeenAcquire = true;
----------------
I think this check is wrong.  (Independent of whether the approach is valid or not.)

My reasoning is that, a cst operation is "at least" an acquire.  However, it's _also_ a release and reordering these two is not legal:
load cst y
store x = 0

This would be a violation of a "release operation" ordering as used by C++11.  It's unclear whether that would violate the LLVM spec as written, but I think it probably should.  

Note that if my reasoning is sound, this is actually a problem with the previous patch as well, not just this one.  


================
Comment at: test/Transforms/DeadStoreElimination/atomic.ll:149
@@ +148,3 @@
+; CHECK: store i32 1
+  store i32 0, i32* @x
+  %x = load atomic i32* @y seq_cst, align 4
----------------
As currently specified in the LangRef, I don't think this is legal.  If I'm reading the spec right, the load must be considered both an Acquire and a Release.  (This is possibly not intended, but seems to follow from the wording.)  As a result, this is not a acquire/release pair, but both a acquire/release and release/acquire pair.  

Finding a case where this is observable would be hard, but it seems possible.  If need be, I'll spend some time thinking about it.  

================
Comment at: test/Transforms/GVN/atomic.ll:46
@@ -45,3 +45,3 @@
 
-; GVN across acquire load (load after atomic load must not be removed)
+; GVN across acquire load (allowed as the original load was not atomic)
 define i32 @test4() nounwind uwtable ssp {
----------------
I don't believe this is valid.

================
Comment at: test/Transforms/GVN/atomic.ll:98
@@ +97,3 @@
+; CHECK: test8
+; CHECK: add i32 %x, %x
+entry:
----------------
This is valid, but not necessarily for the reason you gave.  %y can move above the release.  %x can move below the acquire.  Once this happens, %x & %y can be commoned. 

================
Comment at: test/Transforms/GVN/atomic.ll:110
@@ -85,1 +109,3 @@
+define i32 @test9() nounwind uwtable ssp {
+; CHECK: test9
 ; CHECK: add i32 %x, %x
----------------
CHECK-LABEL please

http://reviews.llvm.org/D4845