Page MenuHomePhabricator

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

Authored by tstellarAMD on May 12 2016, 8:12 AM.

Details

Reviewers
hfinkel
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.

Diff Detail

Event Timeline

tstellarAMD retitled this revision from to GlobalsAA: Don't assume that intrinsics can't access global variables.
tstellarAMD updated this object.
tstellarAMD added a reviewer: hfinkel.
tstellarAMD added a subscriber: llvm-commits.

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)

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.

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 edited edge metadata.May 12 2016, 9:26 AM
hfinkel added a subscriber: hfinkel.
  • Original Message -----

From: "Tom Stellard" <thomas.stellard@amd.com>
To: "thomas stellard" <thomas.stellard@amd.com>, hfinkel@anl.gov
Cc: dberlin@dberlin.org, llvm-commits@lists.llvm.org
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

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.