[PATCH] Relax atomic restrictions on memory dependence analysis

[Philip Reames]

This version looks close to ready.  See inline comments.  Once you fix those, I'll give one more read through before officially giving an LGTM.

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:414
@@ -412,3 +413,3 @@
       // FIXME: This is overly conservative.
-      if (!LI->isUnordered())
-        return MemDepResult::getClobber(LI);
+      if (!LI->isUnordered()) {
+        if (!QueryInst || LI->getOrdering() != Monotonic)
----------------
I originally wrote: "You're still missing the point of my previous example.  The problem is that ordered operations have additional *aliasing* requirements.  Your code is still not checking for these conditions and thus is still incorrect.  "

I believe your code is correct.  You're making a fairly subtle point with your checks though.  Any two potentially aliased monotonic loads are ordered, but a monotonic and unordered load are not.  Even if they alias.  Please clarify this in comments.  I nearly missed it (as you can see from the comment I left in above.)

I would suggest that you change the aliasing specific comment in the same loop.  Keep the comments in sync with the code.  

Nitpick: Separate the first "if (!QueryInst || LI->getOrdering() != Monotonic)" into two clauses.  Semantically, the checks are unrelated.  

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:423
@@ -415,1 +422,3 @@
+            return MemDepResult::getClobber(LI);
+      }
 
----------------
You need to check if LI isVolatile.  LoadInst::isUnordered does this, and you only want to change memory ordering here.  I'd suggest having that be it's own top most check for clarity.

================
Comment at: test/Transforms/DeadStoreElimination/atomic.ll:131
@@ +130,3 @@
+}
+
+; DSE across monotonic load (forbidden since the eliminated store is atomic)
----------------
You could use a couple of other test cases here.  In particular, load-value forwarding across monotonic loads and stores would be helpful.  

A positive example:
store x = 0
store y monotonic
load x <- can use 0 since doesn't participate in ordering even if x == y at runtime

A negative example:
store x = 0 monotonic
load y monotonic
load x monotonic <-- not safe to forward!

And an ambiguous example:
store x = 0 unordered
load y monotonic
load x monotonic <-- is this correct to forward?  I don't know.

http://reviews.llvm.org/D4797