[PATCH] D16857: More detailed memory dependence checking between volatile and non-volatile accesses

[Philip Reames via llvm-commits]

reames added a comment.

Minor code comments inline.  Once addressed should be good to go after quick round of review.

The following is mostly to record my though process...  no response needed and no action required.

The key issue here is one of semantics.  We've been a bit unclear in the past as to whether volatile applied to *memory locations* or *memory accesses*.  At first, it looks like your change is dependent on the location model, but it's not.  You're simply allowing the non-ordered accessed to fall down into the generic may-alias handling below.  This works with both models, but is optional for volatile-locations and required for volatile-access.

If we were going to move towards the volatile-locations model, that's something we'd need to discuss widely on llvm-dev.  I'm mildly in favour of a volatile-access model, but don't know the C++ spec well enough to know what's actually required here.  I'm also of the opinion that if we're going to treat a volatile as applying to *locations* we should consider teaching AA about this fact.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:648
@@ -647,3 +647,3 @@
       // with volatile accesses.
-      if (SI->isVolatile())
-        return MemDepResult::getClobber(SI);
+      if (QueryInst && SI->isVolatile()) {
+        if (LoadInst *QLI = dyn_cast<LoadInst>(QueryInst)) {
----------------
This isn't quite right.  You can't be less precise when lacking information about the querying instruction.  

This needs to be:
if (SI->isVolatile()) {
  if (!QueryInst)
    return a clobber;
  if (!isSimple(QueryInst))
   return a clobber;
}

================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:649
@@ +648,3 @@
+      if (QueryInst && SI->isVolatile()) {
+        if (LoadInst *QLI = dyn_cast<LoadInst>(QueryInst)) {
+          if (!QLI->isSimple())
----------------
Given we've got this same pattern in several places, please pull out a helper function along the lines of "bool isSimple(Instruction* I)".  Could be a lambda, or a static function.  

================
Comment at: test/Transforms/GVN/volatile-nonvolatile.ll:3
@@ +2,3 @@
+; Check that we can eliminate the second load.
+; CHECK: load
+; CHECK-NOT: load
----------------
Stylistic suggestion: put the check lines immediate next to the function, add a CHECK-LABEL: @foo.

================
Comment at: test/Transforms/GVN/volatile-nonvolatile.ll:16
@@ +15,3 @@
+  ret void
+}
+
----------------
Please add a couple of negative tests as well.  


Repository:
  rL LLVM

http://reviews.llvm.org/D16857