[C++20] [Coroutines] Warning for always_inline coroutine

[ChuanqiXu]

Summary

See the discussion in https://reviews.llvm.org/D100282. 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.

This is what GCC does too: https://godbolt.org/z/7eajb1Gf8

[ChuanqiXu]

Format codes

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11110–11113

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?

[ChuanqiXu]

Address comments

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11110–11113

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.

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11110–11113

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.

[ChuanqiXu]

Format codes and edit summary.

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11110–11113

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.

[ChuanqiXu]

@Quuxplusone do you feel good with the current message?

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11110–11113

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.

[ChuanqiXu]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11110–11113

change the compiler's behavior to just quietly do the thing the user is asking for.

This is pretty hard and it isn't hopeful to be done in near future. There are many other TODOs in coroutine and my list.

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.)

The key question here is that I can't think about what the user is asking for. Some people would ask for to inline the just-the-first-segment-of-the-coroutine to enable CoroElide (inline the ramp function is a necessary condition to do coro-elision now). And some other people who are tuning performance would require to inline the whole function. (In the area of tuning benchmark, it is possible that the performance increase 10% after we inlined some hot functions).

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.

Yeah, I would try to document it in https://clang.llvm.org/docs/AttributeReference.html#always-inline-force-inline after we get in consensus. The behavior would be:
(1) Without optimization, nothing would be inlined.
(2) With optimization, only the ramp function would be guaranteed to be inlined. (The other part may get inlined to if situation is simple enough so that the compiler could convert the indirect call to direct call. But I don't want to tell users about this.)

[ChuanqiXu]

Address comments:

• Update warning message as Mathias's suggestion.
• Update document.

[ChuanqiXu]

Here is my thought:

• Should we warn for this situation? Yes, on the on hand, it behaves differently with normal users imaged. A assistant reason would be that GCC would warn for the case too.
• Is the warning message good? I think it at least not worse than what GCC does in this case. Secondly, I think most users wouldn't and shouldn't know about the implementation details of coroutine like ramp functions and so on. It is not true that I don't want tell things precisely. But precision brings burden to users, which is what I want to avoid.

@Quuxplusone @aaron.ballman (I saw you are added as reviewer by herald script) any thought?

[ChuanqiXu]

@Quuxplusone @aaron.ballman gentle ping~

[• Quuxplusone]

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.

clang/include/clang/Basic/AttrDocs.td

6140–6142 ↗

(On Diff #397835)

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.)

clang/include/clang/Basic/DiagnosticSemaKinds.td

11109

clang/lib/Sema/SemaCoroutine.cpp

1065–1070

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.

[ChuanqiXu]

Address comments

[ChuanqiXu]

In D115867#3270137, @Quuxplusone wrote:

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.

Thanks for helping to reword! This idea is inspired by @rjmccall so I guess this one should be good to him. Given the current situation, it might not be easy to find someone else to look at it. I would wait for 1~2 days before landing.

clang/include/clang/Basic/AttrDocs.td

6140–6142 ↗

(On Diff #397835)

I have made a longer explanation at: https://github.com/llvm/llvm-project/issues/53413 (and I cite this in following comment). I feel it is not good to write something so long in this document.

[ChuanqiXu]

Apply Aaron's opinion in: https://github.com/llvm/llvm-project/issues/53413#issuecomment-1024910619.

clang/include/clang/Basic/DiagnosticGroups.td

def CompoundTokenSplit : DiagGroup<"compound-token-split",
                                    [CompoundTokenSplitByMacro,
                                     CompoundTokenSplitBySpace]>;
 def CoroutineMissingUnhandledException :
   DiagGroup<"coroutine-missing-unhandled-exception">;
 def DeprecatedExperimentalCoroutine :
   DiagGroup<"deprecated-experimental-coroutine">;
 def DeprecatedCoroutine :
   DiagGroup<"deprecated-coroutine", [DeprecatedExperimentalCoroutine]>;
-def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException, DeprecatedCoroutine]>;
+def AlwaysInlineCoroutine :
+  DiagGroup<"always-inline-coroutine">;
+def Coroutine : DiagGroup<"coroutine", [CoroutineMissingUnhandledException, DeprecatedCoroutine, AlwaysInlineCoroutine]>;
 def ObjCBoolConstantConversion : DiagGroup<"objc-bool-constant-conversion">;
 def ConstantConversion : DiagGroup<"constant-conversion",
                                    [BitFieldConstantConversion,
                                     ObjCBoolConstantConversion]>;
 def LiteralConversion : DiagGroup<"literal-conversion">;
 def StringConversion : DiagGroup<"string-conversion">;
 def SignConversion : DiagGroup<"sign-conversion">;
 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_coroutine_handle_address_invalid_return_type : Warning <
   "return type of 'coroutine_handle<>::address should be 'void*' (have %0) in order to get capability with existing async C API.">,
   InGroup<Coroutine>;
 def err_coroutine_promise_final_suspend_requires_nothrow : Error<
   "the expression 'co_await __promise.final_suspend()' is required to be non-throwing"
 >;
 def note_coroutine_function_declare_noexcept : Note<
   "must be declared with 'noexcept'"
 >;
+def warn_always_inline_coroutine : Warning<
+  "a coroutine would be splitted into pieces and not every piece of coroutine "
+  "would be guaranteed to be inlined"
+  >,
+  InGroup<AlwaysInlineCoroutine>;
 } // end of coroutines issue category
 
 let CategoryName = "Documentation Issue" in {
 def warn_not_a_doxygen_trailing_member_comment : Warning<
   "not a Doxygen trailing comment">, InGroup<Documentation>, DefaultIgnore;
 } // end of documentation issue category
 
 let CategoryName = "Nullability Issue" in {

clang/lib/Sema/SemaCoroutine.cpp

void Sema::CheckCompletedCoroutineBody(FunctionDecl *FD, Stmt *&Body) {
   if (!Fn->CoroutinePromise)
     return FD->setInvalidDecl();
 
   if (isa<CoroutineBodyStmt>(Body)) {
     // Nothing todo. the body is already a transformed coroutine body statement.
     return;
   }
 
+  // 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.
+  if (FD->hasAttr<AlwaysInlineAttr>())
+    Diag(FD->getLocation(), diag::warn_always_inline_coroutine);
+
   // Coroutines [stmt.return]p1:
   //   A return statement shall not appear in a coroutine.
   if (Fn->FirstReturnLoc.isValid()) {
     assert(Fn->FirstCoroutineStmtLoc.isValid() &&
                    "first coroutine location not set");
     Diag(Fn->FirstReturnLoc, diag::err_return_in_coroutine);
     Diag(Fn->FirstCoroutineStmtLoc, diag::note_declared_coroutine_here)
             << Fn->getFirstCoroutineStmtKeyword();

clang/test/SemaCXX/coroutines.cpp

struct missing_await_resume {
   void await_suspend(std::coroutine_handle<>);
 };
 
 void test_missing_awaitable_members() {
   co_await missing_await_ready{}; // expected-error {{no member named 'await_ready' in 'missing_await_ready'}}
   co_await missing_await_suspend{}; // expected-error {{no member named 'await_suspend' in 'missing_await_suspend'}}
   co_await missing_await_resume{}; // expected-error {{no member named 'await_resume' in 'missing_await_resume'}}
 }
+
+__attribute__((__always_inline__))
+void warn_always_inline() { // expected-warning {{a coroutine would be splitted into pieces and not every piece of coroutine would be guaranteed to be inlined}}
+  co_await suspend_always{};
+}