GlobalsAA: InaccessibleMemOnly does not mean ReadNone.

[vaivaswatha]

Summary

In AnalyzeCallGraph, if we treat InaccessibleMemOnly the same as ReadNone,
the function will end up having "FMRB_DoesNotAccessMemory". This will lead
to situations such as LICM moving out / eliminating calls such as malloc
(in the future, since malloc does not have InaccessibleMemOnly set yet)
from loops.
Instead, treat InaccessibleMemOnly such that the analysis still maintains
info for the function, but keeps it conservative as ModRef.

Depends on D15605

[vaivaswatha]

Ping !

[reames]

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.

lib/Analysis/GlobalsModRef.cpp

519

Removing the check here is definitely called for.

528

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.

test/Analysis/GlobalsModRef/modreftest.ll

1 ↗

(On Diff #43300)

Please separate this into it's own test file since it relies on speculation rather than forwarding to illustrate problems.

[vaivaswatha]

Updating comment and test case as per review comment.

[vaivaswatha]

In D15665#319247, @reames wrote:

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.

I understand we need to be careful while using this attribute. So far, GlobalsModRef is the only analysis that actually uses it. "inaccessiblememonly" does access memory (not visible in the module) and it does cause memory dependence. However, this dependence is not ignored. Adding the check (in this change) just ensures that GlobalsAA maintains info for the function and does not immediately discard it. I had mistakenly added it in the other place in an earlier commit and am correcting that now.

[reames]

Let me repeat my early comment in a slightly stronger form. Unless you have strong evidence that this needs to be in GlobalsAA, please start by implementing this in BasicAA. This will get you most of the properties you're looking for (value forwarding), without the complexity involved in GlobalAA. In particular, teach getModRefInfo(CS, Location) about it.

I also believe that the original GlobalAA patch should be reverted rather than patched in this manner. It was flat out wrong.

[hfinkel]

In D15665#320043, @reames wrote:

Let me repeat my early comment in a slightly stronger form. Unless you have strong evidence that this needs to be in GlobalsAA, please start by implementing this in BasicAA. This will get you most of the properties you're looking for (value forwarding), without the complexity involved in GlobalAA. In particular, teach getModRefInfo(CS, Location) about it.

I also believe that the original GlobalAA patch should be reverted rather than patched in this manner. It was flat out wrong.

I agree with Philip. We need to teach BasicAA about the attribute; only then, if necessary, should we teach GlobalsAA about it. Please revert the GlobalsAA patch, and then, propose a new clean patch for GlobalsAA, if necessary, after the BasicAA work is done.

lib/Analysis/GlobalsModRef.cpp

for (unsigned i = 0, e = SCC.size(); i != e && !KnowNothing; ++i) {
       Function *F = SCC[i]->getFunction();
       if (!F) {
         KnowNothing = true;
         break;
       }
 
       if (F->isDeclaration()) {
         // Try to get mod/ref behaviour from function attributes.
-        if (F->doesNotAccessMemory() || F->onlyAccessesInaccessibleMemory()) {
+        if (F->doesNotAccessMemory()) {
           // Can't do better than that!
         } else if (F->onlyReadsMemory()) {
           FI.addModRefInfo(MRI_Ref);
           if (!F->isIntrinsic())
             // This function might call back into the module and read a global -
             // consider every global as possibly being read by this function.
             FI.setMayReadAnyGlobal();
         } else if (F->onlyAccessesArgMemory() || 
+                   F->onlyAccessesInaccessibleMemory() ||
                    F->onlyAccessesInaccessibleMemOrArgMem()) {
-          // This function may only access (read/write) memory pointed to by its
-          // arguments. If this pointer is to a global, this escaping use of the
+          // This function may at most access (read/write) memory pointed to by its
+          // arguments (i.e., it may not read any globals in this module directly). 
+          // If an argument points to a global, this escaping use of the
           // pointer is captured in AnalyzeUsesOfPointer().
           FI.addModRefInfo(MRI_ModRef);
         } else {
           FI.addModRefInfo(MRI_ModRef);
           // Can't say anything useful unless it's an intrinsic - they don't
           // read or write global variables of the kind considered here.
           KnowNothing = !F->isIntrinsic();
         }

test/Analysis/GlobalsModRef/modreftest2.ll

+; RUN: opt < %s -basicaa -globals-aa -gvn -licm -S | FileCheck %s
+
+@X = internal global i32 4
+
+declare void @InaccessibleMemOnlyFunc( ) #0
+
+attributes #0 = { inaccessiblememonly }
+
+define i32 @test3( ) {
+entry:
+  br label %for.body7
+
+for.body7:
+; CHECK: for.body7
+; CHECK: call void @InaccessibleMemOnlyFunc()
+  %XB = bitcast i32* @X to i1*
+  %x = load i1, i1* %XB
+  call void @InaccessibleMemOnlyFunc( )
+  br i1 %x, label %for.body7, label %for.end12
+
+for.end12: 
+  ret i32 0
+}
+
+