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

[Connor Abbott via Phabricator via llvm-commits]

cwabbott added a comment.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D54516