stabilize optimize attribute#157273
Conversation
|
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 |
|
r? @JohnTitor rustbot has assigned @JohnTitor. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
deb33c2 to
1de7123
Compare
This comment has been minimized.
This comment has been minimized.
1de7123 to
56db805
Compare
|
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 |
This comment has been minimized.
This comment has been minimized.
|
Marking as S-blocked given the current status. |
This commit stabilizes the `#[optimize]` attribute, which allows for more fine-grained control of optimization levels.
56db805 to
680ec3c
Compare
|
These commits modify Please ensure that if you've changed the output:
|
|
With my 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Suggestion: I was looking at the test coverage, do we have any //@ run-pass smoke tests for testing that the attribute E2E-wise doesn't trivially "blow up"? (It obviously will not prove the attribute and its effects work in all possible cases, but a smoke test is still valuable). E.g. this might just be a main that calls a function annotated with #[optimize].
There was a problem hiding this comment.
Question (non-blocking): hm, do we know what happened to optimize-attr-2.rs 🙈
|
I believe the overarching question in stabilizing this attribute should be whether we're happy with the capability and scope of this attribute. Code contained in functions with I realize that I'm basically reiterating the following comment: #54882 (comment). I don't necessarily think that we must block the stabilization of these attributes on this point, but I think it is important that we set ourselves' and the users' expectations on what this attribute can and cannot do. It might very well end up the case that in process of doing so we realize this feature does not pass the bar of quality we expect out of Rust. At the same time this feature is genuinely useful despite all its flaws and those flaws aren't likely to be addressed in any meaningful way, so stabilizing it might be the only way forward for it. |
There was a problem hiding this comment.
Question: this is more from UX / language design PoV, but consider this example:
#[optimize(speed)]
fn outer() {
fn inner() {}
}- Does
#[optimize(speed)]'s hint "effect" apply toouterbut notinner? Should it apply to both?
(I don't think there's test coverage for this)
There was a problem hiding this comment.
Question: currently, it seems that e.g.
const _: () = {
#[optimize(speed)]
fn outer() {
}
};is accepted. What is the expected effect in this case?
(I don't think there's test coverage for this)
There was a problem hiding this comment.
Question: it seems that one can also do
#[optimize(speed)]
const fn foo() {}What is the expected effect in this case?
(I don't think there's test coverage for this)
In general, optimization-controlling attributes don't have strictly defined semantics already, i.e. |
|
From the LLVM perspective, stabilizing these should be fine. I've done some work (culminating in llvm/llvm-project#191363) to ensure that One thing I would like to see more explicitly discussed is the meaning of |
|
☔ The latest upstream changes (presumably #157303) made this pull request unmergeable. Please resolve the merge conflicts. |
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
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
RFC history
Answers to unresolved question
Not yet.
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)#[optimize]is applied to an incompatible item #128458Nightly uses
The standard library itself uses this attribute in a few places
Implementation
Major part
rust/compiler/rustc_codegen_llvm/src/attributes.rs
Line 416 in c0bb140
#[optimize]is applied to an incompatible item #128458optimize(none): Add#[optimize(none)]#128657#[optimize(none)]implies#[inline(never)]#136358 (includes disabling MIR opts)Coverage
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.