[PATCH] D124363: [Coroutines] Don't optimize readnone function before we split coroutine (4/5)

Thu Apr 28 02:21:10 PDT 2022

ChuanqiXu added inline comments.

================
Comment at: clang/lib/CodeGen/CGCall.cpp:2132
     if (TargetDecl->hasAttr<ConstAttr>()) {
-      FuncAttrs.addAttribute(llvm::Attribute::ReadNone);
+      if (!LangOpts.Coroutines)
+        FuncAttrs.addAttribute(llvm::Attribute::ReadNone);
----------------
ChuanqiXu wrote:
> rjmccall wrote:
> > This is a global setting and will affect every function in every file that has coroutines enabled, which is presumably every file compiled with `-std=c++20` or later.  So you're radically changing optimization for a ton of C++ code that doesn't use coroutines.
> > 
> > It should be fine to just add `coro_readnone` instead of `readnone` to call sites from coroutine bodies, right?  Oh, but I guess we could have a `readnone` call in an inlined function body.  Can we just suppress this kind of code motion in coroutine bodies, or is that too invasive to the optimizer?
> > 
> > Actually, maybe your change to this patch was a misunderstanding.  What I was trying to say in Discourse was that I wasn't sure that your early pass approach would work for thread-local variables, because addresses of thread-locals are LLVM constants and can get moved around implicitly; you need the frontend to be involved there so that there's something hooked into the right place in the emitted function.  But I think it should work for this `readnone` annotation.
> > Actually, maybe your change to this patch was a misunderstanding. What I was trying to say in Discourse was that I wasn't sure that your early pass approach would work for thread-local variables, because addresses of thread-locals are LLVM constants and can get moved around implicitly; you need the frontend to be involved there so that there's something hooked into the right place in the emitted function. But I think it should work for this readnone annotation.
> 
> Yeah... I misunderstood your words. You were talking about TLS variable only but I thought you mentioned both.
> 
> > This is a global setting and will affect every function in every file that has coroutines enabled, which is presumably every file compiled with -std=c++20 or later. So you're radically changing optimization for a ton of C++ code that doesn't use coroutines.
> 
> Yeah... it is a **little** better in current version since it affects codes with coroutines only. So it wouldn't affect codes compiled with `-std=c++20` or later if they don't use coroutines. For optimizations, I was imaging we could re-enable the optimization by running EarlyCSE pass after CoroCleanup pass in https://reviews.llvm.org/D124364. But as you said, the optimization would be **changed** in the proposal.
> 
> > Can we just suppress this kind of code motion in coroutine bodies, or is that too invasive to the optimizer?
> 
> Yes. According to the previous discussion (https://discourse.llvm.org/t/rfc-coroutine-and-pthread-self/56985), people don't like the direction to insert checks for coroutines around the passes. It is a burden for other developers to understand and remember they may handle a coroutine.
> 
> > It should be fine to just add coro_readnone instead of readnone to call sites from coroutine bodies, right? Oh, but I guess we could have a readnone call in an inlined function body.
> 
> I am trying to do that. I think we could replace `coro_readnone` with `readnone` when we are inlining a call into an unlowered coroutine. However, I meet a technical problem that it doesn't work if we don't remove `readnone` for the function declaration...
> I would mark this patch as plan changes before I could get a conclusion if it is possible.
> 
> ---
> 
> I think we could go to https://reviews.llvm.org/D124361 first, it doesn't depend on this one and it wouldn't meet the optimization changing problem.
> replace coro_readnone with readnone

replace readnone with coro_readnone

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

https://reviews.llvm.org/D124363