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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

> Sorry, that was a bit of a jumble of different ideas that I pressed into the form of a single message.  Let me try to do better.

I replied to the first post. Let me also reply here ;)

> Effects summarization attributes like `readnone` and so on have to be understood as encompassing the entire behavior of the call.  That of course includes any callbacks into visible code the call might make.  So the absence of `nocallback` doesn't weaken other attributes in any way.

Yes, the entire behavior of the call. To be precise. right now it is the entire "becomes visible as part of the call" behavior and not the "local behavior" of the call.

> I think the right way of thinking about synchronization, effects-wise, is that it is effectively a potential store to any memory that might be accessible by another thread.

That is what we basically do now, unfortunately. It puts all possible effects by any possible other thread together, which is basically always "worthless".

> Synchronization is therefore necessarily excluded by `readonly`, `argmemonly`, and so on.

No. We cannot derive either for "may-sync" functions but the user can for sure combine them in a sensible way. If all my shared state is the one argument I pass, `argmemonly` and "may-sync" work together fine. If I know there is no write effect by other threads that can become visible at this point, e.g., I only wake them up but they haven't done anything on shared state yet, I can use `readonly` with "may-sync", etc.

> We can usually ignore the possibility of stores happening concurrently on other threads, but synchronization forces us to honor them, and that is indistinguishable, in terms of effects, from the store happening directly as part of the call.

Yes. Though with our modeling we can not express that certain effects are known to be not present. We can also not annotate functions based on their local effects and then use the context in which they are called to determine if there are other threads of execution that it might interact with.

> I haven't read much about `nocallback`, so I don't entirely understand what the optimization goal is.

We use it in-tree for inter-procedural reachability analysis. So far, we mostly ask if a load/store may reach another load/store inter-procedurally. If I have `nocallback` I can get proper results because otherwise I have to assume that things like intrinsics and also known runtime functions are potentially calling any extern user function in my module and thereby reach the load/store in question.

> If it's not implied by the existing effects attributes, then I guess we need to add it everywhere, and if that implies test churn, so be it.

Attributes should, in general, not imply other attributes except if we have good reason for it. They compose and that is as it should be.
As an example, I can annotate my `malloc` and `vprintf` calls in the OpenMP GPU runtime as `nocallback` (via `__attribute__((leaf))`) and then perform proper reachability analysis.

> But I want to make sure that we're not modeling something useless in the optimizer just because we can imagine code that it accurately describes; ultimately, the IR is a tool that exists to be of use to us as compiler developers, and perfect accuracy by itself is not a goal.

I agree. But we need to avoid justifying things with the fact that we didn't used in the past. For the longest time we had no parallelism-aware optimizations at all, which allows us to lump synchronization and memory effects together w/o any drawback. We also did not dinstinguish other "non-memory effects", e.g., termination. The latter causes all sorts of hassle for people until we finally decoupled termination from "may-write". I mean, we literally introduced the `llvm.sideffect` intrinsic to pretend there was a write in a loop that may not actually have one. That kind of solution is not helpful either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123531