[PATCH] D54516: [AMDGPU] Do not mark llvm.amdgcn.set.inactive as IntrNoMem

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

In https://reviews.llvm.org/D54516#1301013, @cwabbott wrote:

> In https://reviews.llvm.org/D54516#1300997, @nhaehnle wrote:
>
> > In https://reviews.llvm.org/D54516#1298178, @cwabbott wrote:
> >
> > > I believe the combination of Convergent + not Speculatable should mean that the compiler shouldn't hoist it to a non-control-equivalent block and shouldn't CSE it. In particular, IIRC it's not guaranteed that that a readnone function always return the same value when it's called with the same arguments, so it's not safe to CSE -- it just means that LLVM can move other things across it, since it doesn't modify *caller-visible* state. What pass is causing a problem? Maybe it's a bug in the pass?
> >
> >
> > `readnone` is literally readnone. Maybe you're mixing this up with `inaccessiblememonly`?
>
>
> No, I meant `readnone`, since AFAIK that's what IntrNoMem here maps to. The bit about "caller-visible state" was taken directly from the langref entry for readnone. My point was that in something like:
>
>   WWM code
>   if (cc) {
>     other stuff v1
>   } else {
>     identical WWM code
>     other stuff v2
>   }
>
>
> it wouldn't be allowed for LLVM to rewrite the use of the second WWM to point to the first, even now, since the semantics of `readnone` aren't strong enough to guarantee that the two calls return the same thing, at least as far as I understand (I think someone else, maybe Tom, explained this to me over IRC a while ago). Your issue is different, and indeed removing IntrNoMem isn't going to help with that at all. I agree that we need a better solution for that. But for the current patch, I can't think of a situation where removing NoMem/readonly would disallow a transform that shouldn't be allowed.


Both EarlyCSE and GVN seem to disagree with that, at least for individual function calls marked `readnone`.

@tpr The relevant code in Mesa is here: https://gitlab.freedesktop.org/mesa/mesa/blob/master/src/amd/common/ac_llvm_build.c#L367


Repository:
  rL LLVM

https://reviews.llvm.org/D54516