Enable MIR inlining by cjgillot · Pull Request #91743 · rust-lang/rust

cjgillot · 2021-12-10T12:24:10Z

#82280 has shown nice compile time wins could be obtained by enabling MIR inlining.
Most of the issues in #81567 are now fixed,
except the interaction with polymorphization which is worked around specifically.

I believe we can proceed with enabling MIR inlining in the near future
(preferably just after beta branching, in case we discover new issues).

Steps before merging:

figure out the interaction with polymorphization;
figure out how miri should deal with extern types;
silence the extra arithmetic overflow warnings;
remove the codegen fulfilment ICE;
remove the type normalization ICEs while compiling nalgebra;
tweak the inlining threshold.

rust-highfive · 2021-12-10T12:24:12Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2021-12-10T12:24:14Z

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

cjgillot · 2021-12-10T23:02:12Z

@bors try @rust-timer queue

rust-timer · 2021-12-10T23:02:14Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-12-10T23:02:24Z

⌛ Trying commit 6a4e632dd082da698d23955f33781a45a00d2e64 with merge e158c01eec3c90aaf0463f93c68c9971a0c00924...

compiler/rustc_const_eval/src/interpret/eval_context.rs

bors · 2021-12-11T00:23:23Z

☀️ Try build successful - checks-actions
Build commit: e158c01eec3c90aaf0463f93c68c9971a0c00924 (e158c01eec3c90aaf0463f93c68c9971a0c00924)

rust-timer · 2021-12-11T00:23:24Z

Queued e158c01eec3c90aaf0463f93c68c9971a0c00924 with parent 0b42dea, future comparison URL.

rust-timer · 2021-12-11T03:20:26Z

Finished benchmarking commit (e158c01eec3c90aaf0463f93c68c9971a0c00924): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Large improvement in instruction counts (up to -2.1% on incr-patched: empty 3072 builds of issue-46449)
Very large regression in instruction counts (up to 15.7% on full builds of deeply-nested-async)

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 led 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

cjgillot · 2021-12-11T11:25:30Z

@bors try @rust-timer queue

rust-timer · 2021-12-11T11:25:31Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2021-12-11T11:25:38Z

⌛ Trying commit f5a5f1c979c645cef188e0ca113d427e8dd400be with merge 82c0b57dc36973c6b38f4dcfa748798e4d019ecd...

bors · 2021-12-11T12:59:11Z

☀️ Try build successful - checks-actions
Build commit: 82c0b57dc36973c6b38f4dcfa748798e4d019ecd (82c0b57dc36973c6b38f4dcfa748798e4d019ecd)

rust-timer · 2021-12-11T12:59:12Z

Queued 82c0b57dc36973c6b38f4dcfa748798e4d019ecd with parent 4a66a70, future comparison URL.

rust-timer · 2021-12-11T15:15:35Z

Finished benchmarking commit (82c0b57dc36973c6b38f4dcfa748798e4d019ecd): comparison url.

Summary: This change led to very large relevant mixed results 🤷 in compiler performance.

Large improvement in instruction counts (up to -3.0% on incr-patched: empty 3072 builds of issue-46449)
Very large regression in instruction counts (up to 17.0% on full builds of deeply-nested-async)

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 led 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

library/core/tests/num/wrapping.rs

matthiaskrgr · 2022-07-01T17:14:52Z

Hmm this is not showing up in the bors queue for some reason 🤔
@bors r=oli-obk

bors · 2022-07-01T17:14:54Z

📌 Commit cbbf06b has been approved by oli-obk

bors · 2022-07-02T11:24:20Z

⌛ Testing commit cbbf06b with merge 0075bb4...

bors · 2022-07-02T14:06:20Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 0075bb4 to master...

rust-timer · 2022-07-02T22:23:41Z

Finished benchmarking commit (0075bb4): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	1.3%	7.2%	54
Regressions 😿 (secondary)	1.7%	6.1%	68
Improvements 🎉 (primary)	-2.6%	-10.0%	118
Improvements 🎉 (secondary)	-3.4%	-17.3%	76
All 😿🎉 (primary)	-1.4%	-10.0%	172

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	2.8%	13.5%	105
Regressions 😿 (secondary)	2.8%	4.6%	61
Improvements 🎉 (primary)	-5.8%	-13.4%	23
Improvements 🎉 (secondary)	-4.9%	-5.7%	3
All 😿🎉 (primary)	1.3%	13.5%	128

Cycles

Results

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	2.8%	5.7%	5
Regressions 😿 (secondary)	2.4%	3.4%	16
Improvements 🎉 (primary)	-5.4%	-13.8%	87
Improvements 🎉 (secondary)	-7.2%	-19.0%	37
All 😿🎉 (primary)	-5.0%	-13.8%	92

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

nnethercote · 2022-07-02T22:51:10Z

It's worth pointing out the improvements to compiler bootstrapping:

A: 701.356	B: 639.547	Total: -61.8 (-8.813%)

Impressive!

therealprof · 2022-07-04T15:26:29Z

stm32f4-0.14.0 | opt | full | 7.16% | 37.94x

Boooooo. 😅

I guess tons of trivial code, all inlineable, is a total nightmare for the MIR inliner.

bjorn3 · 2022-07-18T12:24:56Z

This has improved runtime performance of cg_clif compiled programs by a lot (when compiled in release mode). For example rustc compiled using cg_clif now builds the standard library in 8 min instead of 22 min. Thanks everyone who worked on enabling this!

RalfJung · 2022-07-22T18:42:08Z

src/test/codegen/mem-replace-direct-memcpy.rs

+// CHECK: ; core::mem::replace
 // CHECK-NOT: call void @llvm.memcpy
-// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %src, i{{.*}} 1, i1 false)
+// CHECK: call void @llvm.memcpy.{{.+}}({{i8\*|ptr}} align 1 %{{.*}}, {{i8\*|ptr}} align 1 %dest, i{{.*}} 1, i1 false)


This change made the test fail in --stage 1 (the default for ./x.py test) on my system

/home/r/src/rust/rustc.3/build/x86_64-unknown-linux-gnu/test/codegen/mem-replace-direct-memcpy/mem-replace-direct-memcpy.ll:138:2: note: possible intended match here call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %_4, i8* align 1 %src, i64 1, i1 false) ^

It should be src, not dest, it seems. Or it should be %{{.*}} I guess since the name seems to be unstable. The 2nd argument is the src though so the "dest" you added here makes no sense to me.

rust-highfive assigned jackh726 Dec 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2021

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 10, 2021

RalfJung reviewed Dec 10, 2021

View reviewed changes

compiler/rustc_const_eval/src/interpret/eval_context.rs Outdated Show resolved Hide resolved

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Dec 11, 2021

cjgillot force-pushed the enable_mir_inlining_inline_all branch from 6a4e632 to 8f5c54e Compare December 11, 2021 10:16

This comment has been minimized.

Sign in to view

cjgillot force-pushed the enable_mir_inlining_inline_all branch from 8f5c54e to f5a5f1c Compare December 11, 2021 10:36

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 11, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Dec 11, 2021

Aaron1011 reviewed Dec 12, 2021

View reviewed changes

library/core/tests/num/wrapping.rs Outdated Show resolved Hide resolved

This was referenced Jul 2, 2022

Shorten def_span of closures to just their header #98482

Merged

Fix ICE regarding infinite recursive type inference ( Issue -> #92470 ) #98613

Closed

Reword "Required because of the requirements on the impl of ..." #98807

Merged

matthiaskrgr mentioned this pull request Jul 2, 2022

ICE during mir inlining: None in compiler/rustc_mir_transform/src/inline.rs:185:32 #98821

Closed

ghost mentioned this pull request Jul 3, 2022

Recent nightly started ICEing with "No counters provided the source_hash for used function" #98833

Closed

Alexhuszagh mentioned this pull request Jul 17, 2022

ICE with rustc 1.64.0-nightly (27eb6d701 2022-07-04) running on x86_64-unknown-linux-gnu Alexhuszagh/minimal-lexical#14

Closed

This was referenced Jul 18, 2022

Tracking Issue for mir-inlining #81567

Open

Enable mir inlining by default rust-lang/rustc_codegen_cranelift#805

Closed

RalfJung reviewed Jul 22, 2022

View reviewed changes

RalfJung mentioned this pull request Jul 22, 2022

./x.py test src/test/codegen fails #99619

Closed

Amanieu mentioned this pull request Jul 28, 2022

compiler-builtins CI fails on powerpc64 #99853

Closed

andjo403 mentioned this pull request Aug 1, 2022

Suboptimal codegen when using unwrap_or_else with unreachable_unchecked #98468

Closed

This was referenced Aug 7, 2022

ICE while building nalgebra with mir-opt-level 3: ErrorReported in rustc_mir/src/transform/inline/cycle.rs #82650

Closed

ICE in src/test/ui/issues/issue-74614.rs if MIR inlining is enabled #81788

Closed

yanchen4791 mentioned this pull request Aug 9, 2022

Add inline-llvm option for disabling/enabling LLVM inlining #100293

Merged

pnkfelix mentioned this pull request Aug 26, 2022

disable MIR inlining on beta-1.64 #101004

Closed

wesleywiser mentioned this pull request Oct 7, 2022

1.65.0 release notes #102659

Merged

workingjubilee mentioned this pull request Nov 28, 2022

Being stuck on Rust 1.61.0 is unacceptable pgcentralfoundation/plrust#127

Closed

nnethercote mentioned this pull request Mar 2, 2023

Compiler Performance Tracking Issue #48547

Open

clubby789 mentioned this pull request Apr 24, 2023

Match blocks generate inefficient assembly compared to if/else chain since rust 1.65.0 #110737

Open

vlad20012 mentioned this pull request May 21, 2023

[WIP] Test performance of running MIR inliner on inline(always) function calls in optimized incremental build #111804

Closed

mqudsi mentioned this pull request May 20, 2024

3-way comparison is branchier after 1.71 #125338

Open

Uh oh!

Conversation

cjgillot commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Dec 10, 2021

Uh oh!

rust-highfive commented Dec 10, 2021

Uh oh!

This comment has been minimized.

This comment has been minimized.

cjgillot commented Dec 10, 2021

Uh oh!

rust-timer commented Dec 10, 2021

Uh oh!

bors commented Dec 10, 2021

Uh oh!

Uh oh!

bors commented Dec 11, 2021

Uh oh!

rust-timer commented Dec 11, 2021

Uh oh!

rust-timer commented Dec 11, 2021

Uh oh!

This comment has been minimized.

cjgillot commented Dec 11, 2021

Uh oh!

rust-timer commented Dec 11, 2021

Uh oh!

bors commented Dec 11, 2021

Uh oh!

bors commented Dec 11, 2021

Uh oh!

rust-timer commented Dec 11, 2021

Uh oh!

rust-timer commented Dec 11, 2021

Uh oh!

Uh oh!

matthiaskrgr commented Jul 1, 2022

Uh oh!

bors commented Jul 1, 2022

Uh oh!

bors commented Jul 2, 2022

Uh oh!

bors commented Jul 2, 2022

Uh oh!

rust-timer commented Jul 2, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

nnethercote commented Jul 2, 2022

Uh oh!

therealprof commented Jul 4, 2022

Uh oh!

bjorn3 commented Jul 18, 2022

Uh oh!

RalfJung Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

cjgillot commented Dec 10, 2021 •

edited

Loading

RalfJung Jul 22, 2022 •

edited

Loading