[PATCH] Relax atomic restrictions on memory dependence analysis

[Philip Reames]

It feels like you're trying to go too far too fast here.  Given the sensitivity of the topic, I'd would *strongly* request you split this into smaller chunks.  I see several separate optimizations here:
1) "monotonic" does not imply dependence if the addresses are known not alias -- note: your current change doesn't seem to implement the second part of that, which is required for correctness.  
2) "release" does not imply dependence unless there is a following "acquire"
3) the addition of the atomic ops appears to be addressing a bug?  (i.e. do atomics not participate in the ordering at all today?)  If so, this should *definitely* be separate.  

As a general point, you're clearly thinking in terms of acquire and release fences here.  In C++, atomic and release apply to *operations* and as a result, all ordering is respect to specific addresses.  It is conservatively correct to order with respect to all addresses, but not required.  Just pointing that out.

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:374
@@ +373,3 @@
+
+  // Atomic accesses are only a problem if there is an acquire after a release.
+  // See "Compiler testing via a theory of sound optimisations in the C11/C++11
----------------
A more general statement of this might be: "Ordered (atomic) accesses only need to be preserved if their presence or lack thereof are observable according to the memory model.  Based on the results in <paper> we know that ... are observable while ... are not."

I would suggest spelling out the reasoning about why the implied optimization is correct, not the cases where optimizing wouldn't be.  (Well, you can and should state both.)  As a reviewer, I need that justification to assess your design.  

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:421
@@ +420,3 @@
+      // Monotonic accesses are not dangerous for this purpose.
+      if ((!LI->isUnordered()) && LI->getOrdering() != Monotonic)
+        HasSeenAcquire = true;
----------------
This doesn't look right.  

I could understand that a monotonic load wouldn't be a dependence, but shouldn't an cst_seq one still have the clobber behaviour?  

Also, why would seeing an "release" trigger HasSeenAcquire?  

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:481
@@ -474,1 +480,3 @@
+      // Monotonic accesses are safe for this purpose.
+      if (HasSeenAcquire && (!SI->isUnordered()) && SI->getOrdering() != Monotonic)
         return MemDepResult::getClobber(SI);
----------------
I think this could actually be made stronger.  If the ordering on the store is a release, but you haven't seen a require, is the following non-atomic load dependent?  It doesn't seem like it should be.

http://reviews.llvm.org/D4797