Page MenuHomePhabricator

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

Authored by ChuanqiXu on Dec 16 2021, 2:30 AM.

Details

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

Diff Detail

Event Timeline

ChuanqiXu requested review of this revision.Dec 16 2021, 2:30 AM
ChuanqiXu created this revision.
ChuanqiXu updated this revision to Diff 395837.Dec 22 2021, 3:43 AM

Format codes

Quuxplusone requested changes to this revision.Dec 22 2021, 10:08 AM
Quuxplusone added a subscriber: Quuxplusone.
Quuxplusone added inline comments.
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?

This revision now requires changes to proceed.Dec 22 2021, 10:08 AM
ChuanqiXu updated this revision to Diff 395954.Dec 22 2021, 6:11 PM

Address comments

ChuanqiXu added inline comments.Dec 22 2021, 6:31 PM
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.

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 updated this revision to Diff 395959.Dec 22 2021, 7:06 PM
ChuanqiXu edited the summary of this revision. (Show Details)

Format codes and edit summary.

ChuanqiXu edited the summary of this revision. (Show Details)Dec 22 2021, 7:07 PM
ChuanqiXu added inline comments.Dec 22 2021, 7:16 PM
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.

@Quuxplusone do you feel good with the current message?

Quuxplusone requested changes to this revision.Jan 4 2022, 6:34 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
11110–11113

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

This revision now requires changes to proceed.Jan 4 2022, 6:34 AM
ChuanqiXu added inline comments.Jan 4 2022, 6:02 PM
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 updated this revision to Diff 397835.Jan 6 2022, 3:09 AM

Address comments:

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

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?

Quuxplusone accepted this revision.Jan 25 2022, 11:05 AM

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
6208–6210

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
11126
clang/lib/Sema/SemaCoroutine.cpp
1084–1089

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.

This revision is now accepted and ready to land.Jan 25 2022, 11:05 AM
ChuanqiXu updated this revision to Diff 403121.Jan 25 2022, 8:04 PM

Address comments

ChuanqiXu marked 2 inline comments as done.Jan 25 2022, 8:09 PM

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
6208–6210

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.

This revision was automatically updated to reflect the committed changes.