[PATCH] D115867: [C++20] [Coroutines] Warning for always_inline coroutine

Tue Jan 4 06:34:52 PST 2022

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11110-11113
+def warn_always_inline_coroutine : Warning<
+  "A coroutine marked always_inline might not be inlined properly."
+  >,
+  InGroup<AlwaysInlineCoroutine>;
----------------
ChuanqiXu wrote:
> Quuxplusone wrote:
> > ChuanqiXu wrote:
> > > Quuxplusone wrote:
> > > > FWIW, this message isn't particularly helpful to the reader. My code "might" not be optimized "properly"? Does that mean it might be mis-optimized, improperly, and thus break in some way at runtime? Or is the compiler just saying that the attribute will be ignored? Or that it //might// be ignored, but it might not? How do I (the programmer) know whether the bad thing will happen?
> > > > 
> > > > I think as a programmer what I'd //like// to see here is just `"the '%0' attribute has no effect on coroutines"`. That's very clear, and easy to understand. Does that wording reflect what the compiler actually //does//, though?
> > > Thanks for suggestion. The actual behavior isn't easy to describe and understand. Since a coroutine would be splitted into pieces. And the original function would be reduced and we would call it 'ramp function' in compiler. And the call to other new functions would be indirect call. The compiler couldn't inline indirect call. But the compiler **might** convert the indirect call into direct call so that they could be inlined.
> > > 
> > > In summary, the actual behavior might be described as: "Only the ramp function are guaranteed to be inlined and the other new functions may or may not get inlined". But the term "ramp funciton" is used in compiler only (Some guys in LWG/LEWG know it too). And I believe the term shouldn't leak to other users. So I chose the current description. 
> > > 
> > > BTW, I thought the fact that coroutine would be splitted should be transparent to users too. This is the reason why I wrote previous message. But your words make sense. And I couldn't find methods to make it more clear and don't tell the user about coroutine splitting.
> > IIUC, `"this coroutine will be split into pieces; only the first piece will be inlined"` or simply `"the '%0' attribute applies to only the initial piece of this coroutine"`. Possible synonyms for "piece" include "section", "segment", "chunk". Is there any standardese for "the run of stuff in between two suspension points"?
> > 
> > However, I stand by my initial comment that this message is not helpful to the programmer. It's warning me that something bad will happen, right? Instead of having that bad thing happen, why don't you just make the compiler //ignore the attribute// in this situation?
> > 
> > If your answer is "Because always-inlining the initial piece isn't always bad; maybe the programmer thinks it's //good//, and //wants// it to happen," then this shouldn't be a `warning` at all; it should just be documented in the attribute's documentation. Warnings should be for bad/unintentional things, not for things someone might do on purpose.
> >  Is there any standardese for "the run of stuff in between two suspension points"?
> 
> AFAIK, there is no standard terms for it.
> 
> ---
> 
> Very Sorry, I made a mistake in previous comment. The behavior for always-inline ramp function should be: "The ramp function is guaranteed to get inlined with optimization turned on." It implies that ramp function wouldn't get inlined in O0. This is what I am trying to do in: https://reviews.llvm.org/D115790. The current behavior for always-inline coroutine in O0 would be a crash. Here is an example: https://godbolt.org/z/zssKxTPM5.
> 
> And GCC would warn and ban for the always-inline coroutine too: https://godbolt.org/z/7eajb1Gf8. (I understand that it isn't a good argument to say GCC did so. Just a information sharing)
> 
> --- 
> 
> > Warnings should be for bad/unintentional things, not for things someone might do on purpose.
> 
> My point is that the behavior and semantic is inconsistent. The programmer might think the whole coroutine would be inlined. However, it is not the case. I think it is worth a warning.
>>! In D115867#3218653, @ChuanqiXu wrote:
> @Quuxplusone do you feel good with the current message?

No, it's definitely still ungrammatical English, so it shouldn't ship in this state.
Also, I think my entire previous comment stands — both the suggestions for improving the English (without much changing the meaning), and my higher-level suggestion that you should just change the compiler's behavior to //just quietly do the thing the user is asking for//.

If you think the user is really asking to inline just-the-first-segment-of-the-coroutine, then the compiler should inline the first segment of the coroutine (and not warn, because the user's code is correct).

Vice versa, if you think the user is //not// asking to inline just-the-first-segment-of-the-coroutine, then //the compiler should not do that.// (I.e., you should ignore the attribute instead.)

Either way, we should end up with a clear mental model of what this attribute does for coroutines, and that model should be documented in the docs.

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

https://reviews.llvm.org/D115867