Stabilize optimize attribute

[veluca93]

View all comments

This commit stabilizes the #[optimize] attribute, which allows for more fine-grained control of optimization levels.

Stabilization report

Summary

Tracking issue: #54882
Reference PRs: rust-lang/reference#2278

cc @rust-lang/lang @rust-lang/lang-advisors @rust-lang/t-compiler

What is stabilized

// Instructs the optimization pipeline to prioritize smaller code size over speed.
#[optimize(size)]
pub fn binary_size_sensitive() {
    // ...
}

// Instructs the optimization pipeline to prioritize execution speed over code size.
#[optimize(speed)]
pub fn performance_critical() {
    // ...
}

// Disables optimizations entirely for this item.
#[optimize(none)]
pub fn debug_only() {
    // ...
}

What isn't stabilized

We might want to allow specifying a per-function optimization level (i.e. #[optimize(level = 3)]). To my understanding, backend support for this is not quite there yet (and this - or other - extensions have consequently not been implemented yet).

Applying the attribute on a module level is also not implemented yet.

Design

Reference

• Description of the new attribute: Stabilize #[optimize] attribute reference#2278

RFC history

• https://rust-lang.github.io/rfcs/2412-optimize-attr.html

Answers to unresolved question

• Should we also implement optimize(always)? optimize(level=x)?

Not yet.

• Should there be any way to specify what global optimization for speed level is used in conjunction with the optimization for speed option (e.g. -Copt-level=s3 could be equivalent to -Copt-level=3 and #[optimize(size)] on the crate item).

Not yet.

Post-RFC changes

The optimize(none) variant was added. The variants above were discussed here: #54882 (comment)

Key points

• #[optimize(none)] implies #[inline(never)] to be effective (#[optimize(none)] should imply #[inline(never)] #136329)
• As with all optimization-controlling flags, the effects on speed and binary size can be hard to predict: Tracking issue for RFC 2412, "The optimize attribute" #54882 (comment)
• Applying the attribute to incompatible items emits an error: Emit an error if #[optimize] is applied to an incompatible item #128458

Nightly uses

The standard library itself uses this attribute in a few places

Implementation

Major part

• Wiring it up to the llvm codegen backend:

  rust/compiler/rustc_codegen_llvm/src/attributes.rs

  Line 416 in c0bb140

  match codegen_fn_attrs.optimize {

• Catching usage on incompatible items: Emit an error if #[optimize] is applied to an incompatible item #128458
• Implementing optimize(none): Add #[optimize(none)] #128657 #[optimize(none)] implies #[inline(never)] #136358 (includes disabling MIR opts)

Coverage

• Basic tests: https://github.com/rust-lang/rust/blob/c0bb140a37c81cf59a0b40c21c9413059644e294/tests/ui/attributes/optimize.rs
• Testing the effect on compilation: https://github.com/rust-lang/rust/blob/c0bb140a37c81cf59a0b40c21c9413059644e294/tests/codegen-llvm/optimize-attr-1.rs

Tool changes

Trivial changes to rust-analyzer to support the new attribute as an inert attribute: rust-lang/rust-analyzer#22511

Type system, opsem

Breaks the AM?

As far as the AM is concerned, this attribute doesn't exist.

Acknowledgments

Most of the work was not done by me, I'm just writing the stabilization report :-)

@nagisa did the initial implementation, @clubby789 implemented optimize(none) and fixed the attribute being allowed in too many places.

Note

Concerns (0 active)

• are-we-happy-wiht-capability-and-scope-of-this-attribute resolved in this comment
• meaning-of-optimize-size resolved in this comment
• explicitly-decide-on-effect-on-nested-functions resolved in this comment

Managed by @rustbot—see help for details.

[rustbot]

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

[rustbot]

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 17 candidates

[tgross35]

Given the interactions here, I expect this will need a lang+compiler FCP

@rustbot label +I-lang-nominated +I-compiler-nominated

See some recent discussion about this feature on Zulip #t-lang > status of #[optimize] attribute

[JohnTitor]

Marking as S-blocked given the current status.

[rustbot]

These commits modify tests/rustdoc-json.
rustdoc-json is a public (but unstable) interface.

Please ensure that if you've changed the output:

• It's intentional.
• The FORMAT_VERSION in src/librustdoc-json-types is bumped if necessary.

cc @aDotInTheVoid, @obi1kenobi

[obi1kenobi]

With my cargo-semver-checks hat on: do we expect this attribute to have SemVer implications?

For example, can applying or removing this attribute on an item influence the contexts in which that item may be used without compile errors or other changes that would be considered public-API-breaking?

[veluca93]

With my cargo-semver-checks hat on: do we expect this attribute to have SemVer implications?

For example, can applying or removing this attribute on an item influence the contexts in which that item may be used without compile errors or other changes that would be considered public-API-breaking?

I don't believe this is possible - the attribute should only influence how the compiler optimizes functions, and compiler optimizations should not have any visible effects.

[jieyouxu]

Discussion: hm, I wonder if this attribute deserves an entry in the rustc book (a bit like lint levels), because while sure there's the Reference PR, but this is more like a compiler knob?

View changes since the review

[veluca93]

@rfcbot concern should-apply-to-closures

I think this attribute should apply to closures. Not doing so would make sense if we viewed closures as standalone anonymous function objects, but I think it is better to view them as callable extensions of the current function. In this view, code inside a closure is "part of" the enclosing function.

My proposal is different from how #[inline] applies to closures today. But on reflection, I think it would be a mistake to base the behavior of every new codegen attribute on inline, especially when it comes to closures. The inline hint is framed around the mechanics of the function call boundary itself, and its implications are sensitive to the context of caller and callee as well as the compiler and backend implementations. It is not obvious that the inline setting for a function should be applied to a closure, especially in the case of inline(never).

optimize, on the other hand, has a much smoother applicability relationship, and we can say that what applies to an outer function should likely apply to a closure. We have a stable precedent in #[target_feature], which applies to closures in a function. I think coverage (not yet stable) should behave similarly. What all of these have in common is that they apply to the function body rather than the call boundary.

See #158901.

[fee1-dead]

@traviscross said above:

I still expect this attribute to affect only the item to which it is applied, in the same way as inline and our other codegen attributes. We treat closures as a kind of pseudo-item for this purpose.

I am thus marking the linked PR as S-waiting-on-team since there appears to be disagreement on whether closures should be affected.

[traviscross]

I am thus marking the linked PR as S-waiting-on-team since there appears to be disagreement on whether closures should be affected.

For my part, I'm OK with applying this to nested closures. I see the appeal: reducing the degree to which things change when you move code into a closure. Separately, afterward, I'd like us to work to make our existing codegen attributes consistent with this new system. And I'd like us to find a way to fix the hygiene issue when macros introduce attributed closures.

One question: do we want optimize(none) to imply inline(never), or no? This came up as a question when @tmandry and I discussed. Doing so is in tension with the framing that we propagate codegen attributes that apply to the body but not to the function boundary. I'd lean toward separating these back out (though I understand there would remain coupling at the LLVM level). It seems OK to me if optimize(none) code ends up inlined and subject to the optimization level of what it's inlined into.

[traviscross]

We talked about this in the lang call today. @tmandry will give a fuller write-up. In summary, we do not want to specify optimize(none) as implying inline(never), but, of course, the compiler is always free to not inline.

[tmandry]

In summary, we do not want to specify optimize(none) as implying inline(never), but, of course, the compiler is always free to not inline.

That's right. To summarize the model the lang team liked:

• optimize sets the optimization goal of a function body, and closure bodies are part of the body of their containing function.
• inline sets the inline policy of a function call boundary, and closures have their own boundary.
• Neither should imply the other at the language level.
• The compiler is free to change how it inlines to better implement the optimization goal, or vice versa. Both attributes are best-effort, and the implementation approach is allowed to change or ignore the attributes altogether.

If a particular combination of attributes is ineffective, given how our compiler or backend work, we should probably lint on that combination. I don't think we need to block stabilization on such a lint.

[nagisa]

Inheritance of this attribute was also a part of the original rfc that introduced the attribute: https://github.com/rust-lang/rfcs/blob/master/text/2412-optimize-attr.md#reference-level-explanation (4th paragraph.)

[traviscross]

Inheritance of this attribute was also a part of the original rfc that introduced the attribute: https://github.com/rust-lang/rfcs/blob/master/text/2412-optimize-attr.md#reference-level-explanation (4th paragraph.)

(It was left as an unresolved question.)

[scottmcm]

@rfcbot concern optimize-none-not-being-none-muddles-motivation

Ralf pointed out in #128657 (comment) that the PR implementing optimize(none) actually force-enabled some optimization passes that can remove UB.

If it's not "none" (and I guess also different from opt-level=0 or that change wouldn't have been needed?) but "well, sorta none but also optimized enough that operations on ZSTs don't make it to codegen (and maybe more things)", now I'm puzzled what the point of it is.

Did people actually want optimize(debug) or something?

[veluca93]

@rfcbot concern optimize-none-not-being-none-muddles-motivation

Ralf pointed out in #128657 (comment) that the PR implementing optimize(none) actually force-enabled some optimization passes that can remove UB.

If it's not "none" (and I guess also different from opt-level=0 or that change wouldn't have been needed?) but "well, sorta none but also optimized enough that operations on ZSTs don't make it to codegen (and maybe more things)", now I'm puzzled what the point of it is.

Did people actually want optimize(debug) or something?

FWIW I am significantly more interested in optimize(size) and optimize(speed) -- enough that I'd be happy to skip stabilizing optimize(none) for now.

(And among those, I moreover care more about optimize(size))

[veluca93]

@rfcbot concern optimize-none-not-being-none-muddles-motivation

Ralf pointed out in #128657 (comment) that the PR implementing optimize(none) actually force-enabled some optimization passes that can remove UB.

If it's not "none" (and I guess also different from opt-level=0 or that change wouldn't have been needed?) but "well, sorta none but also optimized enough that operations on ZSTs don't make it to codegen (and maybe more things)", now I'm puzzled what the point of it is.

Did people actually want optimize(debug) or something?

Reading back the comments on the tracking issue, it seems that people do indeed want optimize(debug). I don't have especially strong opinions on the naming, but I will note that optimize(none) matches the C++ name for equivalent functionality (https://clang.llvm.org/docs/AttributeReference.html#optnone, which does not guarantee no optimizations either)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[tmandry]

@rustbot label I-lang-nominated

See #157273 (comment). Nominating for team guidance on whether to ship this as optimize(none), optimize(debug), or leave it out for the current stabilization.

[clubby789]

Ralf pointed out in #128657 (comment) that the PR implementing optimize(none) actually force-enabled some optimization passes that can remove UB.

Just for current context, that issue was actually unrelated to is_required/optimize(none): rust-lang/miri#5226 (comment)
The semantics of optimize(none) are being worked on in #160015 and #160057

[RalfJung]

The semantics of optimize(none)

Yeah so that's the question I was about to bring up here. :)

Should optimize(none) be like -Copt-level=0 or should it be somehow even less optimized than that? Personally it feels to me like -Copt-level=0 makes most sense. (That would be -Zmir-opt-level=1 at the moment but that exact mapping is an unstable implementation detail.)

FWIW the same question comes up for optimize(speed). Is that like -O? (I'm not even sure how that one is currently implemented.)

[rust-bors]

☔ The latest upstream changes (presumably #160102) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.