GlobalsAA: Don't assume that intrinsics can't access global variables

[• tstellarAMD]

Summary

The Gobals Alias Analysis was assuming that intrinsics could never read
or write global variables unless the variable was passed as one of the
arguments to the intrinsic.

I can't find this property of intrinsics documented anywhere, so the
code has been updated to treat intrinsics the same as normal function
calls when analyzing usage of global variables.

This fixes a bug in the AMDGPU backend where this incorrect analysis
was causing to the GVN pass to hoist a load across a barrier intrinsic.

[• dberlin]

So this is an interesting question.
Outside of very specific intrinsics (barriers), or intrinsics passed global
variables, which intrinsics do you believe can affect global variables and
why?

:)

(Because if the answer is "none", we should probably make the ability to
read/write non-passed-in globals an intrinsic property that folks have to
opt-in to)

[• tstellarAMD]

In D20206#428381, @dberlin wrote:

So this is an interesting question.
Outside of very specific intrinsics (barriers), or intrinsics passed global
variables, which intrinsics do you believe can affect global variables and
why?

There probably aren't many besides barriers and maybe some of the cache flushing intrinsics.

:)

(Because if the answer is "none", we should probably make the ability to
read/write non-passed-in globals an intrinsic property that folks have to
opt-in to)

My thinking is that is would be easier to treat intrinsics and functions the same rather than having to add an attribute that makes intrinsics behave like functions. There are already attributes like: readnone, readonly, argmemonly that can be used to specify that an intrinsic won't access globals. It doesn't seem like it is too much trouble to ask people to use those for intrinsics, and it looks like most intrinsics already are using these.

[jmolloy]

I'm not sure about this. Does the barrier intrinsic actually access memory? Is that how we've modelled it? (as clobbering everything?)

This seems like a more general problem then something unique to GlobalsAA.

[hfinkel]

• Original Message -----

From: "Tom Stellard" <[e-mail]>
To: "thomas stellard" <[e-mail]>, [e-mail]
Cc: [e-mail], [e-mail]
Sent: Thursday, May 12, 2016 10:46:33 AM
Subject: Re: [PATCH] D20206: GlobalsAA: Don't assume that intrinsics can't access global variables

tstellarAMD added a comment.

In http://reviews.llvm.org/D20206#428381, @dberlin wrote:

So this is an interesting question.
Outside of very specific intrinsics (barriers), or intrinsics
passed global
variables, which intrinsics do you believe can affect global
variables and
why?

There probably aren't many besides barriers and maybe some of the
cache flushing intrinsics.

:)

(Because if the answer is "none", we should probably make the
ability to

read/write non-passed-in globals an intrinsic property that folks
have to

opt-in to)

My thinking is that is would be easier to treat intrinsics and
functions the same rather than having to add an attribute that makes
intrinsics behave like functions. There are already attributes
like: readnone, readonly, argmemonly

We should at least be checking for these here, especially perhaps argmemonly if the others are handled elsewhere. Please also send a message to llvm-dev, we'll need to make sure that all of the target-specific intrinsics headers are updated to have argmemonly where appropriate. I don't think an effort was ever made to do that.

-Hal

that can be used to specify
that an intrinsic won't access globals. It doesn't seem like it is
too much trouble to ask people to use those for intrinsics, and it
looks like most intrinsics already are using these.

http://reviews.llvm.org/D20206

[• tstellarAMD]

In D20206#428444, @jmolloy wrote:

I'm not sure about this. Does the barrier intrinsic actually access memory? Is that how we've modelled it? (as clobbering everything?)

The barrier intrinsic does not actually access memory. It is more like a memfence, and in order to handle it in an optimal way, we would need to add a memfence/nomfence attribute as described here: https://groups.google.com/forum/#!msg/llvm-dev/KRBnfuQkLNA/8TH5Mb_t_xcJ

However, as I understand, modelling it as clobbering everything is conservatively correct, since it will prevent the compiler from moving loads/stores across it.

This seems like a more general problem then something unique to GlobalsAA.

The test case passes with the other alias analysis pass that I tried, and I grep'd through those pass for 'Intrinsic' and didn't really find any special handling applied to all intrinsics. I'm not sure which other kinds of passes might assume that intrinsics don't access global variables.

lib/Analysis/GlobalsModRef.cpp

for (unsigned i = 0, e = SCC.size(); i != e && !KnowNothing; ++i) {
         } 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 {
           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();
+          KnowNothing = true;
         }
         continue;
       }
 
       for (CallGraphNode::iterator CI = SCC[i]->begin(), E = SCC[i]->end();
            CI != E && !KnowNothing; ++CI)
         if (Function *Callee = CI->second->getFunction()) {
           if (FunctionInfo *CalleeFI = getFunctionInfo(Callee)) {

test/Analysis/GlobalsModRef/intrinsic-global.ll

+; RUN: opt < %s -globals-aa -gvn -S | FileCheck %s
+
+; Check that gvn will not hoist the load past the @llvm.amdgcn.s.barrier
+; intrinsic, which should be assumed to read/write all global variables
+; since it does not have any attributes that describe its interaction with
+; memory.
+
+@global = internal  addrspace(3) global i32 undef, align 4
+@global.1 = internal  addrspace(3) global i32 undef, align 4
+
+; CHECK-LABEL: @widget
+; CHECK: bb5:
+; CHECK-NEXT: call void @llvm.amdgcn.s.barrier()
+; CHECK-NEXT: %tmp6 = load i32, i32 addrspace(3)* @global.1
+define void @widget(i32 %arg, i32 %arg1, i32 %arg2) #0 {
+bb:
+  %tmp3 = icmp eq i32 %arg2, 0
+  br i1 %tmp3, label %bb4, label %bb5
+
+bb4:                                              ; preds = %bb
+  store i32 0, i32 addrspace(3)* @global.1, align 4
+  br label %bb5
+
+bb5:                                              ; preds = %bb4, %bb
+  call void @llvm.amdgcn.s.barrier() #0
+  %tmp6 = load i32, i32 addrspace(3)* @global.1, align 4
+  store i32 %tmp6, i32 addrspace(3)* @global, align 4
+  ret void
+}
+
+; Function Attrs: convergent nounwind
+declare void @llvm.amdgcn.s.barrier() #0
+
+attributes #0 = { convergent nounwind }