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

MCP: Rust-style #[inline] attribute. #56

Closed
crlf0710 opened this issue Sep 19, 2020 · 5 comments
Closed

MCP: Rust-style #[inline] attribute. #56

crlf0710 opened this issue Sep 19, 2020 · 5 comments
Labels
disposition-close The FCP starter wants to close this final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang to-announce Not yet announced MCP proposals

Comments

@crlf0710
Copy link
Member

crlf0710 commented Sep 19, 2020

Proposal

Summary and problem statement

Change the semantics of #[inline] attribute.

  • Currently in Rust #[inline] is following LLVM-style inline policies
    ** #[inline(always)] -> alwaysinline
    ** #[inline] -> inlinehint
    ** N/A -> N/A
    ** #[inline(never)] - neverinline

  • However this is unnecessarily complex and hard to use.

  • It also causes compilation performance issues. (e.g. thousands copies of Option::map in rustc_middle according to cargo-llvm-lines, to ask llvm to examine the inlining possibilities one-by-one)

Motivation, use-cases, and solution sketches

  • Inlining should ideally happen before monomorphization.

  • Inlining should ideally be made as deterministic as possible (respecting user's intention).

  • I propose replacing the semantics of these attributes to:
    ** #[inline(always)] -> do inline, not just a hint (always invoke deterministic inlining before monomorphization), raise a warning or error when failing to do so.
    *** Feedback: It was suggested that this become #[inline(required)], and the check be a lint.
    ** #[inline] -> do inline, not just a hint (always invoke deterministic inlining before monomorphization), fallback to not inlining when failing to do so with good reason TBD: <Language should identify and list the possible reasons.> (and maybe tag as alwaysinline to ask llvm to try again).
    ** N/A -> keeping current behavior here: heuristically determine whether to inline or not, left to compiler internals (maybe invoke deterministic inlining before monomorphization, inlinehint).
    ** #[inline(never)] -> do not inline, not just a hint (do not invoke deterministic inlining before monomorphization, neverinline to stop llvm from doing so too)

  • This will not be a breaking change. There're performance impacts, but hopefully a positive one.

Prioritization

  • This is related to the "Targeted ergonomic wins and extensions" because it improves building experience. It is also relatively not a large change.

  • It was mentioned on zulip that the current implementation MIR-inliner is not fully ready. (Need to recheck the feasibility of using MIR inliner to provide the expected "deterministic inlining before monomorphization" behavior and timeframe approximation.)

Links and related work

  • inline keyword is currently mainly used to workaround ODR in C++ language. It causes confusion to beginners too.

Initial people involved

TBD

What happens now?

This issue is part of the experimental MCP process described in RFC 2936. Once this issue is filed, a Zulip topic will be opened for discussion, and the lang-team will review open MCPs in its weekly triage meetings. You should receive feedback within a week or two.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.


EDIT:

  • edited all mentioning of "MIR-inlining" to "deterministic inlining before monomorphization", since the former is an implementation detail.
  • clarified that for the 'N/A' cause, the semantic is not changed.
  • clarified that it's no longer a hint.
@crlf0710 crlf0710 added T-lang major-change Major change proposal labels Sep 19, 2020
@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2020

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@rustbot rustbot added the to-announce Not yet announced MCP proposals label Sep 19, 2020
@scottmcm
Copy link
Member

scottmcm commented Sep 20, 2020

Procedurally, I think this needs some tweaks. From a lang perspective, #[inline] is just a hint (an implementation can completely ignore it) and thus even when used has no impact on observable behaviour.

I would thus say that "we should run a MIR inliner" is entirely a compiler question -- whether or not you do, vs getting LLVM to do it, should be semantically invisible and thus not need lang approval.

So I think the only thing left under lang preview in the proposal is whether to lint on #[inline(always)] that doesn't? (Or potentially stronger alternatives, like some attribute that would guarantee a particular semantic by giving hard errors if it doesn't happen.) If so, I would suggest focusing down to that.


A specific thing to clarify: "N/A -> heuristically determine whether to inline or not, left to compiler internals" appears to have been the case at least as far back as Rust 1.4.0. So it's unclear to me what this issue is proposing to change in that specific bullet.

@crlf0710
Copy link
Member Author

@scottmcm Thanks! I've got the text updated according to the initial feedback here and on zulip.

@nikomatsakis nikomatsakis added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Sep 21, 2020
@nikomatsakis
Copy link
Contributor

Update from 2020-09-21:

Some summary of Zulip thread:

  • Without MIR inlining, we can't guarantee inlining on our side (if LLVM doesn't do it).
  • Most recent draft is to have a lint that warns (or errors) if inlining would not occur, which could impose various restrictive possibilities.
  • And there is some sense that adding #[inline(required)] might be a clearer way to force inlining, since it's a clear opt-in to this rule.

Consensus from the meeting:

  • Based on this we feel the right next step is to work on the implementation of MIR inlining, since that seems like a pre-requisite.
  • We would welcome a lint that warns when #[inline(always)] will not result in inlining.
  • We would prefer not to introduce #[inline(required)] unless we are planning to deprecate #[inline(always)] and transition to required on some timeframe. We do not think it would be good to have always and required both exist on stable (undeprecated) indefinitely.

Based on the above, we are going to recommend this MCP be closed because the next steps are all implementation and no RFC is required. Entering final comment period, let us know if you disagree or see a problem with that.

@nikomatsakis nikomatsakis added the disposition-merge The FCP starter wants to merge (accept) this label Sep 21, 2020
@Mark-Simulacrum Mark-Simulacrum added disposition-close The FCP starter wants to close this and removed disposition-merge The FCP starter wants to merge (accept) this labels Sep 21, 2020
@joshtriplett
Copy link
Member

Closing per previous summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close The FCP starter wants to close this final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang to-announce Not yet announced MCP proposals
Projects
None yet
Development

No branches or pull requests

6 participants