-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Add inline-llvm option for disabling/enabling LLVM inlining #100293
Conversation
r? @jackh726 (rust-highfive has picked a reviewer for you, use r? to override) |
5c061b4
to
81ce207
Compare
r? @nnethercote Some comparisons of rustc-perf results from local x86_64 linux machine with and without LLVM inlining controlled with the newly added Instructions:u of all benchmarks with Profile : opt, Scenario : full, Category : Primary
Instructions:u of all benchmarks with Profile : opt, Scenario : full, Category : Primary & Secondary
|
Those results don't seem unexpected to me, but the real question I guess is the performance impact of disabling LLVM inlining -- one way to try to gauge that is to adjust our CI to run without it, and then see how much slower the produced rustc binaries are. My guess is "much slower, >50%", but I don't really know. |
For some context: I suggested this as a good first task for @yanchen4791 to look into, based on an idea from @joshtriplett. In the next day or two I will look at both the compile time and runtime effects. Thanks for filing the PR, @yanchen4791! |
Thanks for advice, @nnethercote ! Tested the
The produced rustc seems running faster than the rustc produced by default (-Z inline-mir=no -Z inline-llvm=yes"). Here are comparisons of 2 versions of rustc's, using raw data of instructions:u of Primary and Secondary benchmarks. With Primary benchmarks, the total combined
instructions:u, Secondary
|
@yanchen4791: Can you update this PR so that |
@nnethercote Sure. Make inline-llvm false by default. How about default of inline-mir ? Currently default value of inline-mir is false. Should it be changed to true for CI? |
Some inline/codegen tests failed after setting inline_llvm default to false. Here is the list:
|
@nnethercote the documentation is wrong. |
With inline_llvm default being set to false, in addition to failures of some test cases of codegen and inline,
|
Since we're quite interested in perf, I don't think that we have to resolve these failures outright. As long as the compiler builds, we can run perf, it doesn't require passing tests. So if you push a version that defaults to |
81ce207
to
7b27cbb
Compare
@Kobzol Sure, compiler builds without any problem. Pushed with inline_llvm defaults to false. |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 7b27cbb1c4403562563661625d7ca2c38f9a4d6a with merge 9e67400a96e1cc2dc2652554e389f59791dc41b3... |
This comment has been minimized.
This comment has been minimized.
☀️ Try build successful - checks-actions |
Queued 9e67400a96e1cc2dc2652554e389f59791dc41b3 with parent aeb5067, future comparison URL. |
Finished benchmarking commit (9e67400a96e1cc2dc2652554e389f59791dc41b3): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Footnotes |
Bootstrap timings reduced by ~30 % when they're not LLVM inlined (but the compiling |
I'm having trouble keeping track of which results are considered valid and what is being measured. I would be grateful if someone made a list summarizing the experiments with links to the result summaries and what they mean. Which stages of the build do bootstrap timings include? I looked for docs on this but couldn't find any. |
If I understand it correctly, the bootstrap timings are gathered from compiling I tried to gather a summary of the results: LLVM inlining disabled for
|
6568ea6
to
0b2b07c
Compare
@nnethercote I've cleaned up testing code and rebased the branch. Also changed the documentation of |
inline_mir: Option<bool> = (None, parse_opt_bool, [TRACKED], | ||
"enable MIR inlining (default: no)"), | ||
"enable MIR inlining (default: off)"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No other options use default: off
, so I think this should stay as default: no
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why inline_mir
is an Option<bool>
rather than just a bool
. The comment on parse_opt_bool
says:
/// Use this for any boolean option that lacks a static default. (The
/// actions taken when such an option is not specified will depend on
/// other factors, such as other options, or target options.)
Seems like inline_mir
has an obvious static value. Anyway, not really important for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, changed back. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that MIR inlining is not on or off by default but depends on other options: whether it's a debug or release build, whether incremental is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then the use of Option<bool>
is good for inline_mir
. Thanks for the clarification.
src/test/rustdoc-ui/z-help.stdout
Outdated
@@ -54,7 +54,8 @@ | |||
-Z incremental-info=val -- print high-level information about incremental reuse (or the lack thereof) (default: no) | |||
-Z incremental-relative-spans=val -- hash spans relative to their parent item for incr. comp. (default: no) | |||
-Z incremental-verify-ich=val -- verify incr. comp. hashes of green query instances (default: no) | |||
-Z inline-mir=val -- enable MIR inlining (default: no) | |||
-Z inline-llvm=val -- enable LLVM inlining (default: yes) | |||
-Z inline-mir=val -- enable MIR inlining (default: off) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
It just needs the |
0b2b07c
to
052887e
Compare
@bors r+ rollup=always |
⌛ Testing commit 052887e with merge c0ec2e0a4576203fb7a9cd9377cb293617939fb7... |
💔 Test failed - checks-actions |
@bors retry |
Rollup of 9 pull requests Successful merges: - rust-lang#100293 (Add inline-llvm option for disabling/enabling LLVM inlining) - rust-lang#100767 (Remove manual <[u8]>::escape_ascii) - rust-lang#101668 (Suggest pub instead of public for const type item) - rust-lang#101671 (Fix naming format of IEEE 754 standard) - rust-lang#101676 (Check that the types in return position `impl Trait` in traits are well-formed) - rust-lang#101681 (Deny return-position `impl Trait` in traits for object safety) - rust-lang#101693 (Update browser UI test 0 10) - rust-lang#101701 (Rustdoc-Json: Add tests for trait impls.) - rust-lang#101706 (rustdoc: remove no-op `#search`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
In this PR, a new -Z option
inline-llvm
is added in order to be able to turn on/off LLVM inlining.The capability of turning on/off inlining in LLVM backend is needed for testing performance implications of using recently enabled inlining in rustc's frontend (with -Z inline-mir=yes option, #91743). It would be interesting to see the performance effect using rustc's frontend inlining only without LLVM inlining enabled. Currently LLVM is still doing inlining no mater what value inline-mir is set to. With the option
inline-llvm
being added in this PR, user can turn off LLVM inlining by using-Z inline-llvm=no
option (the default of inline-llvm is 'yes', LLVM inlining enabled).