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

Tracking issue for opt-level={s,z} #35784

Closed
alexcrichton opened this issue Aug 18, 2016 · 23 comments · Fixed by #50265
Closed

Tracking issue for opt-level={s,z} #35784

alexcrichton opened this issue Aug 18, 2016 · 23 comments · Fixed by #50265
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Added in #32386 these options are still unstable, this issue will track their progress to becoming stable!

@alexcrichton alexcrichton added T-tools T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 18, 2016
@alexcrichton
Copy link
Member Author

Also note that rust-lang/cargo#3007 is adding Cargo support, currently through opt-level = "z" and "s", but as pointed out we may want more human readable names.

@whitequark
Copy link
Member

I don't even think there is a single word that describes what -Oz does. As far as I know it was first introduced in clang and selected for its closeness to -Os and connotations of compression, but this doesn't help at all when you need an immediately understandable token in Cargo.toml.

@matklad
Copy link
Member

matklad commented Aug 19, 2016

size/size-1 for s and size-2 for z?

@Timmmm
Copy link
Contributor

Timmmm commented Oct 24, 2016

@matklad is right, "s" and "z" are not clear at all. I had no idea what "z" even did until I looked it up. Apparently it is the same as "s" but without loop vectorisation. According to the BSD man page it is intended to reduce size even further.

I like the "size-1"/"size-2" idea.

@bholley
Copy link
Contributor

bholley commented Feb 2, 2017

We are potentially interested in this for Gecko's integration of Rust code, see [1]. We'd need to do more measurements to determine if the performance impact is acceptable though.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1328497

@alexcrichton
Copy link
Member Author

@bholley thanks for the info! We haven't seen a lot of movement here in awhile and we've got support in Cargo/build scripts now, so I'd like to propose we stabilize this:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Feb 2, 2017

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

No concerns currently listed.

Once these reviewers reach consensus, 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!

See this document for info about what commands tagged team members can give me.

@nrc
Copy link
Member

nrc commented Feb 3, 2017

IS this properly documented? That can come after we've approved for stability, right?

@alexcrichton
Copy link
Member Author

@nrc good point! I think we should definitely make sure it's documented at least in --help or the various man pages.

@nikomatsakis
Copy link
Contributor

@nrc yes, that should come before the final PR itself lands though.

@wycats
Copy link
Contributor

wycats commented Feb 26, 2017

It seems like there was an open concern that these tokens (especially for Cargo) aren't very clear. Do we want to take a short at single-word tokens before stabilizing?

@alexcrichton
Copy link
Member Author

@wycats I'm not personally motivated to do so as this is such a niche feature it seems like it's not worth the effort, but others may feel differently.

@Timmmm
Copy link
Contributor

Timmmm commented Feb 28, 2017

I feel differently! If I change the patch to size-1, and size-2 would that be acceptable? Or is it too late since Cargo has already merged s and z? If so then we should probably stick with that but they should be properly documented since clearly very few people know what they mean.

@alexcrichton
Copy link
Member Author

@Timmmm I think we're definitely still able to change, we just might have to have a small period of time where we support everything to deprecate s/z

@wycats
Copy link
Contributor

wycats commented Mar 1, 2017

@alexcrichton @Timmmm is there any particular reason that, for Cargo, we need more than just "size"? In practice, the main historical distinction between s and z was that z was OSX-specific, and gave you "definitely, for realz optimize for size."

As far as I can tell, Cargo really only needs a way for people to say "I want you to optimize for size, please" and having multiple different versions of "optimize for size" at the Cargo level seems likely to introduce more confusion than help.

I propose that we add "size" to Cargo, and not leak "s" and "z". I think I feel the same way about rustc but don't have as strong of an opinion there.

@alexcrichton
Copy link
Member Author

Sounds plausible to me!

In that case sounds like there's work to do, so I'll cancel the FCP

@rfcbot fcp cancel

@rfcbot
Copy link

rfcbot commented Mar 1, 2017

@alexcrichton proposal cancelled.

@Timmmm
Copy link
Contributor

Timmmm commented Mar 5, 2017

@wycats That's not what z does, as far as I can tell. According to Clang's manual:

-Oz: Like -Os (and thus -O2), but reduces code size further.

Also see my previous link.

By the way, there is also yet another optimisation setting! -Og, which is:

-Og: Like -O1. In future versions, this option might disable different optimizations in order to improve debuggability.

Sounds like it could be useful. I suggest opt-level = "debug" for that.

@chrivers
Copy link

chrivers commented May 6, 2017

I came here because I wasn't sure what -Oz did - now I know. I would like to point out that "size1" and "size2" is not much better, imho. Ok, so there's a size difference. But which one is supposed to be smaller? The bigger number, because it tries more agressively to make it smaller, or the smaller number, for intuitive reasons?

I can see the parallel with O1, O2, O3 -> increasing optimization for speed, and S1, S2, S3 -> increasing optimization for size, but perhaps this should be made more explicit then? We can always keep "s", "o", and so on as shorthand aliases

@Mark-Simulacrum Mark-Simulacrum added T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue. and removed T-tools labels May 24, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 22, 2017
@vorner
Copy link
Contributor

vorner commented Mar 22, 2018

Hello

This seems to have gone to sleep. I mean, the name for it is important, but no discussion seems to be happening for like 9 months ‒ is it likely to move forward? As for me, I'd rather have the feature stabilized (I'm often aiming at devices with chronic lack of storage space), no matter the actual values used.

Or is it blocked on anything else beside choosing the right name?

@nikomatsakis
Copy link
Contributor

Side note: the crash in #48967 only reproduces with -Os (probably an LLVM bug)

@japaric
Copy link
Member

japaric commented Apr 26, 2018

Since the lastest LLVM upgrade rustc / LLVM does more agressive inlining loop unrolling. This results in increased binary size of embedded / no_std programs: a hundreds of bytes increase, or about a 7x increase, in the case of the smallest Cortex-M binary cf. #49260.

As we are shooting for embedded Rust on stable it would be great to also provide a way to optimize for size (which is pretty important for embedded applications that target resource constrained devices) on stable.

@alexcrichton What can we do to move this forward. Can we at least stabilize "s" on rustc? Cargo already allows opt-level = "s" in the Cargo.toml but -C opt-level=s requires a nightly toolchain.

@aturon is this (opt-level) being discussed as part of the Cargo profile revamp?

@alexcrichton
Copy link
Member Author

@japaric I think those optimizations have baked long enough that it should be fine to stabilize now personally!

japaric added a commit to japaric/rust that referenced this issue Apr 27, 2018
bors added a commit that referenced this issue May 21, 2018
stabilize opt-level={s,z}

closes #35784
closes #47651

### Rationale

Since the lastest LLVM upgrade rustc / LLVM does more agressive loop unrolling. This results in increased binary size of embedded / no_std programs: a hundreds of bytes increase, or about a 7x increase, in the case of the smallest Cortex-M binary cf. #49260.

As we are shooting for embedded Rust on stable it would be great to also provide a way to optimize for size (which is pretty important for embedded applications that target resource constrained devices) on stable.

Also this has been baking in nightly for a long time.

r? @alexcrichton which team has to sign off this?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-dev-tools Relevant to the dev-tools subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.