[PATCH] Relax atomic restrictions on memory dependence analysis

[Robin Morisset]

Improve patch for atomics in MemoryDependenceAnalysis based on Philip Reames comments

1) Erase the third part of the patch (dealing with fences/AtomicRMW) as the current code
    was actually fine after testing (I should have tested this first, mea culpa). Replaced
    with a comment explaining why the current code works, as it depends on a non-obvious part
    of the implementation of AliasAnalysis.
2) Expand the original comment to give the intuition of the paper
3) Expand the comments for load/store, and change the wording of the condition so that it is
    clearer what is tested.

Thanks a lot for the comments !
I can still try to split the patch in two if you want, but the third part is gone, and with the
clearer condition it is unclear how I should split the rest: Monotonic accesses are not treated
really specially.

http://reviews.llvm.org/D4797

Files:
  lib/Analysis/MemoryDependenceAnalysis.cpp

Index: lib/Analysis/MemoryDependenceAnalysis.cpp
===================================================================
--- lib/Analysis/MemoryDependenceAnalysis.cpp
+++ lib/Analysis/MemoryDependenceAnalysis.cpp
@@ -371,11 +371,18 @@
   unsigned Limit = BlockScanLimit;
   bool isInvariantLoad = false;
 
-  // 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
-  //   memory model" in PLDI 2013 for details.
+  // We must be careful with atomic accesses, as they may allow another thread
+  //   to touch this location, cloberring it. Based on the results of
+  //   "Compiler testing via a theory of sound optimisations in the C11/C++11
+  //   memory model" in PLDI 2013, we know that this is only possible if there
+  //   is a release access, followed by an acquire access.
+  // The general idea is that for a discriminating context (i.e. that accesses
+  //   MemLoc) to not be racy, it must synchronize with both the access being
+  //   optimized and the previous one, which requires respectively an acquire
+  //   and a release on this thread.
   // This paper does not prove this result for RMW accesses/fences, so we are
-  // conservative with them.
+  // conservative with them. They are detected as ModRef by the alias analysis,
+  // so this function returns Clobber on them automatically.
   bool HasSeenAcquire = false;
 
   if (isLoad && QueryInst) {
@@ -417,8 +424,10 @@
     // a load depends on another must aliased load from the same value.
     if (LoadInst *LI = dyn_cast<LoadInst>(Inst)) {
       // Atomic loads have complications involved.
-      // Monotonic accesses are not dangerous for this purpose.
-      if ((!LI->isUnordered()) && LI->getOrdering() != Monotonic)
+      // Unordered/Monotonic accesses are not dangerous for this purpose,
+      // and a load cannot be Release or Acquire_Release.
+      if (LI->getOrdering() == Acquire
+              || LI->getOrdering() == SequentiallyConsistent)
         HasSeenAcquire = true;
 
       AliasAnalysis::Location LoadLoc = AA->getLocation(LI);
@@ -477,8 +486,11 @@
 
     if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
       // Atomic stores have complications involved.
-      // Monotonic accesses are safe for this purpose.
-      if (HasSeenAcquire && (!SI->isUnordered()) && SI->getOrdering() != Monotonic)
+      // Unordered/Monotonic accesses are not dangerous for this purpose,
+      // and a store cannot be Acquire or Acquire_Release.
+      if (HasSeenAcquire &&
+              (SI->getOrdering() == Release
+               || SI->getOrdering() == SequentiallyConsistent))
         return MemDepResult::getClobber(SI);
 
       // If alias analysis can tell that this store is guaranteed to not modify
@@ -503,25 +515,6 @@
       return MemDepResult::getClobber(Inst);
     }
 
-    // FIXME We are probably too conservative here.
-    if (isa<FenceInst>(Inst)) {
-      return MemDepResult::getClobber(Inst);
-    }
-
-    // FIXME We are probably too conservative here.
-    if (auto RMWI = dyn_cast<AtomicRMWInst>(Inst)) {
-        if (RMWI->getOrdering() != Monotonic) {
-            return MemDepResult::getClobber(Inst);
-        }
-    }
-
-    // FIXME We are probably too conservative here.
-    if (auto CASI = dyn_cast<AtomicCmpXchgInst>(Inst)) {
-        if (CASI->getSuccessOrdering() != Monotonic) {
-            return MemDepResult::getClobber(Inst);
-        }
-    }
-
     // If this is an allocation, and if we know that the accessed pointer is to
     // the allocation, return Def.  This means that there is no dependence and
     // the access can be optimized based on that.  For example, a load could
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D4797.12263.patch
Type: text/x-patch
Size: 3734 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140807/2d159c6d/attachment.bin>