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

RFC: Add additional inline intents #3778

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented Feb 23, 2025

This proposes adding #[inline(trampoline)] and #[inline(rarely)] to hint the compiler additional information about why you're marking the function for inlining, to help it hopefully make better choices without the overly-strong hammers of always or never.

Rendered

@scottmcm scottmcm added the T-lang Relevant to the language team, which will review and decide on the RFC. label Feb 23, 2025
@scottmcm scottmcm force-pushed the more-inline-options branch from b2c407c to be7ed44 Compare February 23, 2025 01:54
@scottmcm scottmcm force-pushed the more-inline-options branch from be7ed44 to 87ef134 Compare February 23, 2025 02:07
@clarfonthey
Copy link

I like this but am kind of mentally conflicted about the name "trampoline." I can't tell if it's a term that's already in use, or if it's something that was made up to match the metaphor.

Ultimately would be fine with the name and am not going to block this on bikeshedding, but, kind of wanted to express my thoughts in case someone else feels more strongly and isn't sure where others stand.

@kennytm
Copy link
Member

kennytm commented Feb 23, 2025

IIUC after this RFC we would have these 8 inline levels (including the rustc_* ones):

Attribute LLVM function attribute MIR inliner effect
#[rustc_force_inline = "reason"] alwaysinline compile error if cannot inline
#[inline(always)] alwaysinline always inline
#[inline] inlinehint -Zinline-mir-hint-threshold=100
- - -Zinline-mir-threshold=50
#[inline(trampoline)] -? forces caller_is_inline_forwarder?
#[inline(rarely)] -? an even lower threshold?
#[rustc_no_mir_inline] - never inline
#[inline(never)] noinline never inline

@scottmcm
Copy link
Member Author

scottmcm commented Feb 23, 2025

I like this but am kind of mentally conflicted about the name "trampoline." I can't tell if it's a term that's already in use, or if it's something that was made up to match the metaphor.

See https://en.wikipedia.org/wiki/Trampoline_(computing) -- it's used for lots of things. Probably the closest meaning to this one is the calling convention one, where a trampoline is a function with calling convention A that rearranges the stack/registers then calls another function with calling convention B.

If there's a better name, though, I'd be happy to switch it.


@kennytm There might be another if you count implicitly cross-crate-inline as different from #[inline].

We might be able to replace rustc_no_mir_inline with inline(rarely), though -- block it in mir inlining unless all the arguments are Operand::Const, say. That'd be enough for the two cases on my machine in library right now.

(Also, both rustc_no_mir_inlines are also marked #[inline], so I think they're actually LLVM inlinehint.)

Comment on lines +48 to +50
it's a function with a common trivial path, but which sometimes needs to call
out to a more complicated version, like how `Vec::push` is usually trivial but
occasionally needs to reallocate.
Copy link

@hanna-kruppe hanna-kruppe Feb 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big thumbs up for some way of expressing this intent, usually but not always together with hint::cold_path(). Currently the best way to express this is by putting #[cold] or #[inline(never)] on the uncommon, more complicated code path. But both of those options imply some incorrect/undesirable things.

Comment on lines +87 to +89
In LLVM, `#[inline]` sets the [`inlinehint` function attribute](https://llvm.org/docs/LangRef.html#function-attributes),
so `inline(rarely)` could skip doing that, and thus comparatively slightly
discourage inlining it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even without inlinehint LLVM's inlining heuristic can be mostly characterized as "yes", so I'm not sure if I'd describe this behavior as "inline rarely". This is not just a naming concern -- I don't know off-hand where I'd use an attribute that works this way. Not adding any #[inline] attribute and turning on ThinLTO mostly covers the "I don't want to encourage inlining in general but it's fine if it happens" scenarios for me without the costs of emitting multiple copies of the function in different CGUs.

[drawbacks]: #drawbacks

These are still up to the programmer to get right, so
- they might just make analysis paralysis worse
Copy link

@hanna-kruppe hanna-kruppe Feb 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also the proposed inline(usually) (rust-lang/rust#130679) which I think is very well-motivated but adds to this problem. It would be great if we could just make the user-facing inline(always) work that way and keep an internal attribute like #[rustc_force_inline] for the cases (intrinsics) that need inlining even in opt-level=0 builds, but it's not clear if that will work out in the end.

@Jules-Bertholet
Copy link
Contributor

Perhaps inline should work like the diagnostic:: namespace, such that an unrecognized intent is a warning & no-op instead of an error?

@FHTMitchell
Copy link

I feel like the In Combination section could do with a code example - I'm finding it hard to track exactly which attribute would go on what function. Also NonZero::new seems to come out of nowhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants