[PATCH] D89711: [Coroutine] Prevent value reusing across coroutine suspensions in EarlyCSE and GVN

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

> First of all, we cannot drop the readnone tag in the definition of pthread_self in Clang, the regression in the non-coroutine cases are likely unacceptable and they shouldn't pay for it if not using coroutines.

Like I noted before, we can recover the optimization power by special-casing it in alias analysis.  For almost all optimizations, it doesn't matter that it reads memory if that read doesn't actually alias anything.

> Secondly, because one can call pthread_self through indirect function calls, hence just checking for pthread_self in coroutines is not sufficient. Instead, in a coroutine function, we never want to reuse the results of function calls.

This depends on what we decide the "const" attribute actually means, in the context of coroutines.  We could decide that const actually means the value can't depend on the thread ID, then treat fixing up the libc header as a compatibility hack.  Or we could decide that "const" is actually allowed to change across a coroutine suspend, in which case we need to do something more invasive.

Either way, I'd still prefer that the IR readnone exclude functions that behave like pthread_self, I think.

> In all the relevant passes that would reuse call results (EarlyCSE and GVN as far as I know, but do let me know if there are others), do not reuse call results within a coroutine.

We also have a few other GVN-ish passes: NewGVN, GVNHoist and GVNSink. SimplifyCFG does a form of CSE, although I'm not sure it actually affects this case.  LICM also reuses call results in a sense that's relevant here.

Even if we do catch all the cases that are relevant right now, having an "extra" form of memory access that isn't visible to alias analysis makes understanding existing code, and writing new code, harder.

-------

I think this discussion is getting to the point where it would benefit from a wider audience on llvm-dev.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89711