Skip to content
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

#[inline(never)] does not work for async functions #129347

Open
michaelwoerister opened this issue Aug 21, 2024 · 15 comments
Open

#[inline(never)] does not work for async functions #129347

michaelwoerister opened this issue Aug 21, 2024 · 15 comments
Labels
A-async-await Area: Async & Await A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@michaelwoerister
Copy link
Member

I would expect the following code produced a Future::poll() impl with the noinline attribute.

#![feature(noop_waker)]

use std::task::{Context, Waker, Poll};
use std::future::Future;

#[inline(never)]
pub async fn foo() -> u32 {
    std::hint::black_box(1)
}

pub fn bar() -> Poll<u32> {
    let f = std::pin::pin!(foo());
    f.poll(&mut Context::from_waker(Waker::noop()))
}

Instead, the poll method has inlinehint applied to it and gets inlined accordingly:

; This is the function creating the async fn obj:
; Function Attrs: noinline nonlazybind uwtable
define i8 @_RNvCs8Nh0J9OTdDr_15async_fn_inline3foo() unnamed_addr #2 {
  %1 = alloca [1 x i8], align 1
  store i8 0, ptr %1, align 1
  %2 = load i8, ptr %1, align 1
  ret i8 %2
}

; This the poll method:
; Function Attrs: inlinehint nonlazybind uwtable <== this should arguably be noinline too
define internal { i32, i32 } @_RNCNvCs8Nh0J9OTdDr_15async_fn_inline3foo0B3_(...)

Meta

rustc --version --verbose:

rustc 1.82.0-nightly (636d7ff91 2024-08-19)
binary: rustc
commit-hash: 636d7ff91b9847d6d43c7bbe023568828f6e3246
commit-date: 2024-08-19
host: x86_64-unknown-linux-gnu
release: 1.82.0-nightly
LLVM version: 19.1.0
@michaelwoerister michaelwoerister added the C-bug Category: This is a bug. label Aug 21, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 21, 2024
@jieyouxu jieyouxu added A-async-await Area: Async & Await A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-codegen Area: Code generation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue labels Aug 21, 2024
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 21, 2024
@compiler-errors
Copy link
Member

I feel like it's actually up for debate whether #[inline] on an async fn should apply to the function item itself (that produces the future) or the poll function of the coroutine that the item returns. This distinction matters, and it's a subtlety that's not really possible to clearly distinguish with the current inline syntax.

@michaelwoerister
Copy link
Member Author

michaelwoerister commented Aug 21, 2024

I agree that that's not clear. It would be great, however, if there was some way to control the inlining behavior of the poll function.

@compiler-errors
Copy link
Member

I can agree about that!

@traviscross traviscross added the AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. label Oct 3, 2024
@traviscross
Copy link
Contributor

@rustbot labels +T-lang +I-lang-nominated

We discussed this during WG-async triage. On the one hand, this is an optimization, so it seems like a compiler issue. But there's also a lang issue hiding in here:

To what does the attribute apply?

That's probably a lang question. So let's lang nominate it and discuss.

Our feeling on the WG-async call was that the attribute should probably apply semantically to both the function that constructs the future and to the constructed poll function. We felt in particular that the current behavior of it not applying to the poll function is surprising and less useful.

And we felt that if people needed more control than that, the suggestion would be that they desugar the async fn into #[inline(..)] fn .. -> impl Future<..> { #[inline(..)] async { .. }}.

What does the team think?

@rustbot rustbot added I-lang-nominated Nominated for discussion during a lang team meeting. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 3, 2024
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Oct 16, 2024

I said this in the meeting but this is an interesting question that is related to the "flavors" RFC I recently opened. I think it implies that #[inline] should be something you can put on async blocks. I think when you put it on an async function, it should go on the poll (same with applying it to an async closure). The general principle is that this attribute should apply to "where the code is" in all cases.

This suggests that if you REALLY want control, you can desugar like so

#[inline(always)]
async fn foo() -> T { ... }

// becomes

fn foo() -> async -> T {
    #[inline(always)] // <-- this goes here
    async move { ... }
}

This implies you can add attributes on the fn foo explicitly now.

@traviscross
Copy link
Contributor

@rfcbot fcp merge

We talked about this in the lang call today. We want attributes like this to apply to "where the code is", i.e. to the poll function in this case.

The wrapper function, on the other hand, should always be inlined.

@rustbot labels -I-lang-nominated

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 16, 2024
@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 16, 2024
@traviscross traviscross removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 16, 2024
@traviscross
Copy link
Contributor

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Oct 16, 2024

@traviscross proposal cancelled.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 16, 2024
@traviscross
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 16, 2024

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 16, 2024
@nikomatsakis
Copy link
Contributor

@rfcbot reviewed

@rust-lang rust-lang deleted a comment from rfcbot Oct 16, 2024
@tmandry
Copy link
Member

tmandry commented Oct 24, 2024

@rfcbot reviewed

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Oct 24, 2024
@rfcbot
Copy link

rfcbot commented Oct 24, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Oct 24, 2024
@scottmcm
Copy link
Member

The wrapper function, on the other hand, should always be inlined.

I'll note that that's the default, thanks to #116505, so from a lowering perspective nothing should be needed for that. Notable, it should not be inline(always), which would just unnecessarily penalize debug-mode compile-time. (Explicitly #[inline] would be fine, but probably not worth bothering.)

@rfcbot reviewed

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 3, 2024
@rfcbot
Copy link

rfcbot commented Nov 3, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-codegen Area: Code generation A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. C-bug Category: This is a bug. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests