[PATCH] D15665: GlobalsAA: InaccessibleMemOnly does not mean ReadNone.

[Philip Reames via llvm-commits]

reames added a subscriber: reames.
reames added a comment.

I'm really not sure inaccessible memonly belongs in GlobalsAA at all.  Particularly since we haven't added even basis cases to BasicAA.

In general, I think we need to be really careful when implementing this.  It's fair to say that the call doesn't modify or read any *particular* LLVM value, but I'm not sure our aliasing model is correct if we say that it can't read *any* LLVM value.  In particular, as you point out, inaccessiblememonly may still be modifying memory and have memory dependence.


================
Comment at: lib/Analysis/GlobalsModRef.cpp:519
@@ -518,3 +518,3 @@
         // Try to get mod/ref behaviour from function attributes.
-        if (F->doesNotAccessMemory() || F->onlyAccessesInaccessibleMemory()) {
+        if (F->doesNotAccessMemory()) {
           // Can't do better than that!
----------------
Removing the check here is definitely called for.

================
Comment at: lib/Analysis/GlobalsModRef.cpp:528
@@ -527,2 +527,3 @@
         } else if (F->onlyAccessesArgMemory() || 
+                   F->onlyAccessesInaccessibleMemory() ||
                    F->onlyAccessesInaccessibleMemOrArgMem()) {
----------------
Adding it here may not be.  The problem here is that we need to model the memory dependence between two inaccessablememonly functions.  I think the result is correct, but the comment is definitely incorrect for inaccessible memory.  

================
Comment at: test/Analysis/GlobalsModRef/modreftest.ll:1
@@ -1,2 +2,1 @@
-; RUN: opt < %s -basicaa -globals-aa -gvn -S | FileCheck %s
 
----------------
Please separate this into it's own test file since it relies on speculation rather than forwarding to illustrate problems.  


http://reviews.llvm.org/D15665