-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C++20 coroutines marked always_inline
are (partially) inlined at -O1, but not at -O0
#53413
Comments
@llvm/issue-subscribers-c-20 |
always_inline
are (partially) inlined at -O1, but not at -O0
Not sure that filing and closing bugs is maybe the best way to document intended behavior - presumably that sort of thing should go in some documentation somewhere in Clang? (maybe in the documentation for attributes) |
I agree that we should make sure our docs are sufficiently clear on this.
Is there a reason we don't want to be as aggressive? |
@dwblaikie @AaronBallman I put the conclusion at https://reviews.llvm.org/D115867#change-3MBnnJ2r7Yn8. I didn't put the analysis part in the document since I feel it is lengthy and wordy... Given that a coroutine is a special function, which wouldn't take a big part of all functions. It looks odd to me as a user that the document for 'always_inline' talks about
Yeah, there are some reasons: |
I think we should explicitly document the circumstances under which
The whole point to the |
I think the
Yeah, we did give a diagnostic in https://reviews.llvm.org/D115867. Although it is a warning instead of an error. I prefer the warning since that there is an optimization depending on inlining (some part of) coroutine. Yeah, I agree that there is a really mismatch between the behavior and the intention of |
It's for both (we usually mention when an attribute has implementation-detail effects that users generally shouldn't care about. e.g., https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/AttrDocs.td#L430).
It's possible that I'm confused. My understanding of the issue is that when the user writes
Ah, it's great that we give a diagnostic (I don't have a strong opinion on error vs warning), but that's all the more reason why this behavior should be documented. |
Yeah, the actual behavior would be that the Although the Do you accept to add a link to the issue in the AttrDocs? If yes, I would like to add it. If no, I still want to say that the most user of |
No, I don't think the documentation should link to issues (unless the issue is a major one that we're committed to solving and awareness is critical, or some other extenuating circumstance). I don't think we need a wordy or lengthy description here, but I also disagree with "don't care really" either. If we thought it was worth adding a warning over (which I agree was the right move), it's worth documenting the behavior to the user so they understand why they get the warning and what they can or can't do about it. I'm thinking something as simple as:
or something to that effect. |
I've addressed your opinion in https://reviews.llvm.org/D115867. Thanks for helping wording! |
@llvm/issue-subscribers-clang-codegen |
The decision about the issue has been made actually. The intention of the issue is to make it clear in case the user is confused. It would be easier for them to search.
The core problem is about pipeline ordering. In O0 pipeline, the AlwaysInline pass run before CoroSplit pass. And the coroutine haven't been split couldn't be inlined due to the current design of coroutine. So here is the problem.
Note that in On (n != 0) pipeline, the AlwaysInline pass would run after CoroSplit pass.
I guess there would be two question:
(1) Could we make coroutine to be able to be inlined before to be split?
Theoretical possible. But it is hard in practice. Also there are other issues in coroutine now. So I would say it wouldn't be possible in near future.
(2) Could we run CoroSplit pass before AlwaysInline pass?
Maybe yes. The reason why we didn't do this is that LLVM Coroutine is shared by C++ Coroutine, swift coroutine and MLIR for now. The list might be longer in the future (maybe won't). And we need to keep consistent with what other language do here.
Another reason to do the decision is that we don't think this is a key issue. Generally we expect to get performance improvement if we marked a function with always_inline. But if we care about performance, we shouldn't use O0. Also always_inline is not a standard feature, so it shouldn't be a bug if it doesn't work under O0.
If you have a strong reason to inline coroutine with always_inline at O0, you could state your reason and reopen the issue.
BTW, GCC made a more aggressive decision: disable the use of always_inline with coroutine no matter what the optimization level is: https://godbolt.org/z/df8WEj9jb
The text was updated successfully, but these errors were encountered: