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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
