[PATCH] D123531: [GlobalsModRef][FIX] Ensure we honor synchronizing effects of intrinsics

Fri Apr 15 12:19:07 PDT 2022

jdoerfert added a comment.

In D123531#3454415 <https://reviews.llvm.org/D123531#3454415>, @rjmccall wrote:

> `readnone` etc. surely encompass the entire behavior of the call, which of course includes any callbacks it might make.

Agreed, that is what it means now. What the code here assumed is that an intrinsic could *not* callback into the module, but it can.

> And synchronization needs to be understood as effectively a store, since synchronizing with another thread which has done a store guarantees the visibility of that store on this thread; and thus `readonly` etc. must imply `nosync`.

Yes, at least that is the traditional interpretation which is leftover from the time we did not have `nosync`.
So, when we want to document the status quo, then we'd say `nosync` and `read/write/only/none` do *not* compose on the call level but only on the overall effect level.
Thus, `read/write/only/none` are "stronger" than "may-sync", if present. FWIW, we do not have a verifier check for that, e.g., that `read-only/none` has to go with `nosync`.
Instead of creating one we could also change that assumption but that is probably another story.
The current interpretation effectively means our annotations for certain intrinsics (e.g., gpu shuffle) are wrong as they are "must-sync" and `readnone`.

> So whatever `nocallback` means, it must be finer-grained, suitable for situations where the stronger attributes cannot be used.

I don't think that is the right way to look at it. In contrast to `nosync`, `nocallback` is even now orthogonal/composable with memory attributes.
`nocallback` means it cannot invoke module code. It's not otherwise impacting the memory attributes, nor are they impacting `nocallback` beyond their particular specification.
Think of a `readonly` function/intrinsic which is not `nocallback`. It can totally call into the module and use a function there to read a global internal to the module that does not escape.
The function in the module doesn't even need to be `readonly`. It just needs to be `readonly` for the invocations by the `readonly` function/intrinsic.
Even `readnone` composes as it allows the function/intrinsic that is not `nocallback` to call into the module and get the address of a internal global, e.g., in order to return it.

> I think it would be a huge shame if we had to go update a million testcases just to compensate for `nocallback` being added when there's already a stronger attribute on the intrinsic.  That does not seem like a good use of anyone's time.

As I said above, `nocallback` and memory attributes compose already. No need to change anything as far as I can tell. The issue fixed in this patch did also not arise because of their combination. Memory attributes on functions w/ or w/o `nocallback` work the same way.
`nosync` is a different story. We should de-tangle it wrt. memory intrinsics and that would require test changes (among other things). However, if we could simply augment exiting `read/write/only/none` with `nosync`. That's mechanical. We are going to update literally ever test case for opaque pointers, before we did it for explicit load/store types, etc.
De-tangling the effects would hover open up the door for proper reasoning about non-escaped local variables in multi-threaded environments, something we basically cannot do right now. In the current situation "may-sync" prevents `read-only/none`. This is bad now because shuffle intrinics, for example, have (or would need to have) arbitrary side-effects even though they don't need that. If we untangle the attributes we get composing attributes. It would also ensure memory attributes are "local", as the rest of our attributes are. sync/nosync models effects from other threads of execution, there is no need to muddle that with the local read/write behavior of the function at hand.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123531/new/

https://reviews.llvm.org/D123531