[PATCH] D124361: [Coroutines] Add coro_maychange intrinsic to solve TLS problem (2/5)

[John McCall via Phabricator via llvm-commits]

rjmccall added inline comments.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:2623
+      CGF.getLangOpts().Coroutines)
+    V = CGF.Builder.CreateCoroMayChange(V);
+
----------------
I guess this is unnecessary under OpenMP because the privatization logic will already anchor this appropriately in the function.  That's worth mentioning in a comment so that readers don't think the combination is somehow busted.

Same question as the other patch: is there any way to safely only do this in code that's actually going to be part of a coroutine?  Because `getLangOpts().Coroutines` is true for everyone using `-std=c++20`, and most of that code is probably not using coroutines.  It seems like we have two options:
- Use this pattern everywhere in the translation unit, and then eliminate it from all functions after we've split all coroutines
- Use this pattern in the frontend when directly emitting unsplit coroutine bodies, and also change LLVM to introduce this pattern when cloning code into an unsplit coroutine body (most importantly, in the inliner)

I think the second option makes more sense.  In the first option, the mere possibility of having coroutines in the module could significantly impede optimization.  The second option also means you'll have to find and remove this pattern in *all* functions, not just when transforming coroutines.

You could also use the second option for your `readnone` problem: you can have your early pass make `readnone` calls directly from coroutine bodies `coro_readnone` instead, and the inliner can do the same for `readnone` calls being inlined into unsplit coroutines.


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

https://reviews.llvm.org/D124361