Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd CodeGen options to optimize for size. #32386
Conversation
rust-highfive
assigned
arielb1
Mar 21, 2016
This comment has been minimized.
This comment has been minimized.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
|
Are these orthogonal to the Also, I wonder if this could be a level thing too, e.g. |
This comment has been minimized.
This comment has been minimized.
|
I agree with @huonw that we likely want to fold this all into one parameter so we can play around with various values in the future. cc @rust-lang/tools, @rust-lang/compiler |
This comment has been minimized.
This comment has been minimized.
|
Shouldn't this also change inline threshold? See #29943. |
This comment has been minimized.
This comment has been minimized.
|
So, clang specifies Internally in clang -Os and -Oz I think the opt_size integer is an excellent idea, or we could encode the It's up to you all. How do I gate these commands on nightly, there was discussion on irc about Thanks!
|
This comment has been minimized.
This comment has been minimized.
|
It looks like clang's other special treatment of these function attributes is to optimize any |
This comment has been minimized.
This comment has been minimized.
|
Ah yes and I agree with @sanxiyn that we should mirror the inline threshold decisions in clang for now, and if the optimize-for-size flag factors into that we should likely do so too. |
brandonedens
force-pushed the
brandonedens:llvm_min_size
branch
from
b6d13ad
to
16dcdc6
Mar 22, 2016
This comment has been minimized.
This comment has been minimized.
|
I also noticed in clang lib/Frontend/CompilerInvocation.cpp +442 Looks like it also might be appropraite to disable unrolling loops when opt_size > 0. Still poking around in the Rust internals to see how that would be appropriate to do. |
This comment has been minimized.
This comment has been minimized.
|
Looking at |
This comment has been minimized.
This comment has been minimized.
rust-highfive
assigned
alexcrichton
and unassigned
arielb1
Mar 22, 2016
alexcrichton
added
the
T-tools
label
Mar 23, 2016
This comment has been minimized.
This comment has been minimized.
|
This is looking great to me, thanks @brandonedens! We talked in person about perhaps adding a disabling of loop unrolling as well, but otherwise could you just add a test or two to the PR? This could both test acceptance of the option and also perhaps add a |
This comment has been minimized.
This comment has been minimized.
|
Ok, we discussed this at the tools triage meeting yesterday and here were some of our thoughts:
|
This comment has been minimized.
This comment has been minimized.
|
Excellent to hear of the conversation and it was a pleasure to chat the Super excited about this infrastructure work but bear with me; just As I mentioned before it might be prudent to follow a strategy similar to So via GCC you can execute So we have the following meta flags. Clang: I'd probably switch opt-level to mirror the naming in GCC / Clang, a Lately I've been integrating GCC output with other compilers and its really Can we gate this stuff on nightly only? I know you said it'd immediately be Regardless, I'll try to build up something that looks more akin to Clang on Brandon
|
This comment has been minimized.
This comment has been minimized.
|
Yeah we certainly have the ability to make things nightly only if necessary. For example we can have nightly-only compiler flags (which require an unstable opt-in) or we could gate specific values of specific flags (if we really want). It's basically up to do whatever. We do have some |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
trufae
commented
Mar 27, 2016
|
Can you provide some size comparisons compiling (Servo for example) ? |
brandonedens
force-pushed the
brandonedens:llvm_min_size
branch
2 times, most recently
from
219410b
to
8230673
Mar 28, 2016
This comment has been minimized.
This comment has been minimized.
|
I've yet to be able to compile servo with my latest update to the branch. I'll see if I can figure out how to get servo mach to use my local rust build. I did however compile the time example from Iron under optimizations -opt-level=2,3,s,z on x86_64 as --release after I saw your request.
I started looking at the symbol size differences between O3 and Oz which is kinda interesting. Can produce diffs and/or the ELF files if you'd like to look at them yourself. |
This comment has been minimized.
This comment has been minimized.
|
Interesting! Looks like it's a <1% difference between O3 and Oz? |
This comment has been minimized.
This comment has been minimized.
|
Here are the differences between O3 to Oz sorted by size and alphabetic. Note the elements that came from the Rust compile. |
alexcrichton
reviewed
Mar 29, 2016
| llvm::LLVMPassManagerBuilderUseInlinerWithThreshold(builder, t as u32); | ||
| } | ||
| (llvm::CodeGenLevelNone, _) => { | ||
| (_, llvm::CodeGenOptSizeDefault, _) => { | ||
| llvm::LLVMPassManagerBuilderUseInlinerWithThreshold(builder, 75); |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Mar 29, 2016
Member
It looks like this doesn't quite mirror the standard LLVM logic?
If O3 is enabled, the threshold seems to be 275, then there's size/min (75/25), and finally there's the default 225.
This comment has been minimized.
This comment has been minimized.
sanxiyn
Mar 29, 2016
Member
There are 225 and 275 below. Yes, the test is in different order, but opt_level and opt_size are not independent. opt_level is O2 if opt_size is Os or Oz.
This comment has been minimized.
This comment has been minimized.
alexcrichton
Mar 29, 2016
Member
What I'm pointing out is that this is different from clang, which we are intentionally mirroring for now.
This comment has been minimized.
This comment has been minimized.
|
@brson how do you feel about |
brandonedens
force-pushed the
brandonedens:llvm_min_size
branch
from
8230673
to
558eeb9
Apr 2, 2016
brandonedens
force-pushed the
brandonedens:llvm_min_size
branch
from
558eeb9
to
673147d
Apr 19, 2016
brandonedens
added some commits
Mar 27, 2016
brandonedens
force-pushed the
brandonedens:llvm_min_size
branch
from
673147d
to
49d2825
Apr 29, 2016
This comment has been minimized.
This comment has been minimized.
|
Bump Alex. |
This comment has been minimized.
This comment has been minimized.
|
Here's a comparison between two release builds of the iron library with and without optimize for minsize. Might be interesting to see what final binaries look like after building rust proper minsize optimized. Still haven't succeeded in getting rust built from a local rust; advice appreciated. ▶ size target/release-minsize/libiron.rlib ▶ size target/release/libiron.rlib |
This comment has been minimized.
This comment has been minimized.
alexcrichton
reviewed
Apr 29, 2016
| (Some("2"), _) => OptLevel::Default, | ||
| (Some("3"), _) => OptLevel::Aggressive, | ||
| (Some("s"), true) => OptLevel::Size, | ||
| (Some("z"), true) => OptLevel::SizeMin, |
This comment has been minimized.
This comment has been minimized.
alexcrichton
Apr 29, 2016
Member
Could you tweak the error message here to indicate that nightly is required if s or z is passed on the stable/beta builds?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
brandonedens
Apr 30, 2016
Author
Contributor
Yes. I added a early exit for attempts to optimize for size on non-nightly.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
May 2, 2016
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
@bors: retry On Mon, May 2, 2016 at 1:13 PM, bors notifications@github.com wrote:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
merged commit 16eaecb
into
rust-lang:master
May 3, 2016
alexcrichton
referenced this pull request
Aug 18, 2016
Closed
Tracking issue for opt-level={s,z} #35784
chris-morgan
referenced this pull request
Oct 6, 2017
Closed
FAQ describes something from 1½ years ago as “recent” #45061
This comment has been minimized.
This comment has been minimized.
spacekookie
commented
Jan 5, 2018
|
I'm sorry to maybe be asking a stupid question here but how would I go about using |
This comment has been minimized.
This comment has been minimized.
|
@spacekookie it's all good! However, we try to keep the issue tracker soley for bugs; getting help on a PR isn't the best way to go about it. Could you post your question to https://users.rust-lang.org/ instead? It's the best place to ask for questions and get help. Thanks! |
brandonedens commentedMar 21, 2016
Add CodeGen options to annotate functions with the attributes OptimizeSize and/or MinSize used by LLVM to reduce .text size.
Closes #32296