ChuanqiXu added inline comments.
================ 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>; ---------------- 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. 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
