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

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.
This revision is now accepted and ready to land.

LGTM now, modulo my suggested edits (and the necessary corresponding edits in the test case).
I don't think I'm really qualified to accept, but as nobody else is looking, and my name is the one with the red next to it, I'll at least change mine to green. I //recommend// getting someone else to look at this before landing it.



================
Comment at: clang/include/clang/Basic/AttrDocs.td:6140-6142
+Note that a coroutine function wouldn't get inlined in O0 if it is marked with "always_inline".
+In other optimization levels, only the first part of the coroutine - "ramp function"<https://llvm.org/docs/Coroutines.html>`_
+can be guaranteed to be inlined.
----------------
Why is the `-O0` behavior different for coroutines, versus regular functions? My understanding from this documentation is that `-O0 + always_inline` //will// attempt inlining for regular functions, but will //not// attempt inlining (not even of the ramp) for coroutines. That feels asymmetric and weird.
So my default assumption is that maybe this documentation is wrong?
...ah, I see you mention below that this is a known deficiency. So I think this documentation is OK after you take my suggested edit. (I don't know if it would be appropriate to mention the bug number in this documentation as well, so I'll err on the side of not mentioning it.)


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11109
+def warn_always_inline_coroutine : Warning<
+  "a coroutine may be split into pieces and not every piece of the coroutine can be guaranteed to be inlined"
+  >,
----------------



================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1084-1089
+  // The coroutine marked always inline might not be inlined properly in current
+  // compiler support. Since the coroutine would be splitted into pieces. And
+  // the call to resume() and destroy() functions might be indirect call. Also
+  // the ramp function wouldn't get inlined under O0 due to pipeline ordering
+  // problems. It might be different to what users expects to. Emit a warning to
+  // tell it.
----------------
Update the bug number, filing a new bug if needed. That way at least we have a place the reader can go to learn more about what you're talking about.


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

https://reviews.llvm.org/D115867