rust
rust copied to clipboard
rust-lang
Enable MIR inlining in incremental mode too.
Gating MIR inlining on non-incremental mode causes spurious codegen test failures for contributors that do not compile rustc in incremental mode.
This PR stops this behaviour, to rely only on optimization options. In general, release mode disables incremental, so this should not change much for end users.
Fixes https://github.com/rust-lang/rust/issues/99619
r? @jackh726
(rust-highfive has picked a reviewer for you, use r? to override)
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
finished in 3.602 seconds
Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
running 42 tests
Some tests failed in compiletest suite=codegen-units mode=codegen-units host=x86_64-unknown-linux-gnu target=x86_64-unknown-linux-gnu
ii...................i.........FFF.F......
---- [codegen-units] src/test/codegen-units/partitioning/local-inlining-but-not-all.rs stdout ----
These items should have been contained but were not:
These items should have been contained but were not:
MONO_ITEM fn inline::inlined_function @@ local_inlining_but_not_all-inline[External]
thread '[codegen-units] src/test/codegen-units/partitioning/local-inlining-but-not-all.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2771:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
---- [codegen-units] src/test/codegen-units/partitioning/local-transitive-inlining.rs stdout ----
These items should have been contained but were not:
MONO_ITEM fn direct_user::foo @@ local_transitive_inlining-indirect_user[Internal]
MONO_ITEM fn inline::inlined_function @@ local_transitive_inlining-indirect_user[Internal]
thread '[codegen-units] src/test/codegen-units/partitioning/local-transitive-inlining.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2771:13
---- [codegen-units] src/test/codegen-units/partitioning/local-inlining.rs stdout ----
---- [codegen-units] src/test/codegen-units/partitioning/local-inlining.rs stdout ----
These items should have been contained but were not:
MONO_ITEM fn inline::inlined_function @@ local_inlining-user1[Internal] local_inlining-user2[Internal]
thread '[codegen-units] src/test/codegen-units/partitioning/local-inlining.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2771:13
---- [codegen-units] src/test/codegen-units/partitioning/inlining-from-extern-crate.rs stdout ----
---- [codegen-units] src/test/codegen-units/partitioning/inlining-from-extern-crate.rs stdout ----
These items should have been contained but were not:
MONO_ITEM fn cgu_explicit_inlining::always_inlined @@ inlining_from_extern_crate[Internal] inlining_from_extern_crate-mod2[Internal]
MONO_ITEM fn cgu_explicit_inlining::inlined @@ inlining_from_extern_crate[Internal] inlining_from_extern_crate-mod1[Internal]
thread '[codegen-units] src/test/codegen-units/partitioning/inlining-from-extern-crate.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2771:13
causes spurious codegen test failures for contributors that do not compile rustc in incremental mode
Note that it causes failures for people who do, because CI tests with incremental disabled.
Could you add "Fixes #99619" to the issue description? For reference, it's this issue:
- #99619
Won't this slow down compilation for people who compile rustc with incremental enabled? Rustc is always compiled in release mode.
There is a subtle ICE in incremental-ignore-spans mode used by the incremental test suite. This PR is blocked on me fixing that ICE.
:hourglass: Trying commit ffec672af2d26dbdbdddc90e18b6cf2fcda7073e with merge 5a39b2d33adc481de0478cd0c3a78889b2733498...
:sunny: Try build successful - checks-actions
Build commit: 5a39b2d33adc481de0478cd0c3a78889b2733498 (5a39b2d33adc481de0478cd0c3a78889b2733498)
Queued 5a39b2d33adc481de0478cd0c3a78889b2733498 with parent 2f847b81a0d8633f200f2c2269c1c43fe9e7def3, future comparison URL.
Finished benchmarking commit (5a39b2d33adc481de0478cd0c3a78889b2733498): comparison url.
Instruction count
- Primary benchmarks: mixed results
- Secondary benchmarks: mixed results
| mean[^1] | max | count[^2] | |
|---|---|---|---|
| Regressions 😿 (primary) |
6.8% | 52.9% | 41 |
| Regressions 😿 (secondary) |
2.6% | 5.4% | 9 |
| Improvements 🎉 (primary) |
-1.7% | -3.1% | 5 |
| Improvements 🎉 (secondary) |
-1.6% | -2.6% | 3 |
| All 😿🎉 (primary) | 5.9% | 52.9% | 46 |
Max RSS (memory usage)
Results
- Primary benchmarks: 😿 relevant regressions found
- Secondary benchmarks: mixed results
| mean[^1] | max | count[^2] | |
|---|---|---|---|
| Regressions 😿 (primary) |
6.2% | 18.1% | 24 |
| Regressions 😿 (secondary) |
2.7% | 3.6% | 4 |
| Improvements 🎉 (primary) |
-2.6% | -2.6% | 1 |
| Improvements 🎉 (secondary) |
-2.9% | -2.9% | 1 |
| All 😿🎉 (primary) | 5.8% | 18.1% | 25 |
Cycles
Results
- Primary benchmarks: mixed results
- Secondary benchmarks: mixed results
| mean[^1] | max | count[^2] | |
|---|---|---|---|
| Regressions 😿 (primary) |
8.0% | 34.9% | 24 |
| Regressions 😿 (secondary) |
3.8% | 6.0% | 5 |
| Improvements 🎉 (primary) |
-3.2% | -5.5% | 5 |
| Improvements 🎉 (secondary) |
-2.4% | -2.4% | 1 |
| All 😿🎉 (primary) | 6.0% | 34.9% | 29 |
[^1]: the arithmetic mean of the percent change [^2]: number of relevant changes
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 @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.
@bors rollup=never @rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression
The perf impact has to be expected. Perf runs the compiler in incremental+optimizations mode, which is not a typical work case. The faster LLVM optimizations do not happen as often as in the non-incremental case, probably because we don't use the same amount of CGUs in the two cases. The cost centers are:
- loading optimized MIR from incremental cache ;
- writing optimized MIR to incremental cache ;
- writing metadata ;
- collecting monomorphizations.
I conclude that the regression is entirely due to larger MIR. The only workaround would be to tweak MIR inlining threshold for size.
The one thing I'd note about the perf report is that (if I'm reading things correctly) the size of compiled binaries seems to have increased. That seems concerning, because it means we're inlining a lot of things that LLVM is not, which we generally shouldn't be. Lowering the threshold is one path towards fixing this, but we should potentially be revisiting the algorithm for calculating the decision more generally
The original issue has been fixed since the PR was opened, I think because we had a bootstrap bump so both stage 1 and stage 2 are now using MIR inlining. Does it still make sense to keep this PR open?
