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

[WIP] Test performance of running MIR inliner on inline(always) function calls #82280

Closed

Conversation

wesleywiser
Copy link
Member

r? @ghost

@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 19, 2021
@bors
Copy link
Contributor

bors commented Feb 19, 2021

⌛ Trying commit c2230893d572fed6a1809edc67098f5bf08be1c9 with merge 386d02a615f9f2266eeae4d38d62aba0d4b994de...

@jyn514 jyn514 added A-mir-opt-inlining Area: MIR inlining T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 19, 2021
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Feb 19, 2021

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

@rust-timer
Copy link
Collaborator

Queued 386d02a615f9f2266eeae4d38d62aba0d4b994de with parent 0148b97, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (386d02a615f9f2266eeae4d38d62aba0d4b994de): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 19, 2021
@wesleywiser
Copy link
Member Author

A few small wins, lots of significant losses. The main culprit seems to be increased incremental cache load times.

This is probably proof that addressing #80536 will be critical before we see compile time wins from this optimization.

@wesleywiser wesleywiser reopened this Mar 2, 2021
@wesleywiser wesleywiser force-pushed the enable_mir_inlining_inline_all branch from c223089 to 71afdfc Compare March 2, 2021 01:01
@wesleywiser
Copy link
Member Author

@bors try @rust-timer queue

@rust-log-analyzer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 2, 2021
@bors
Copy link
Contributor

bors commented Mar 2, 2021

⌛ Trying commit 71afdfc4d02f8d9b030954134b2242bde5106c7c with merge bd198f8ac3c38a1e932bacccc5a2cdfb569c1972...

@bors
Copy link
Contributor

bors commented Mar 2, 2021

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

@rust-timer
Copy link
Collaborator

Queued bd198f8ac3c38a1e932bacccc5a2cdfb569c1972 with parent 4f20caa, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (bd198f8ac3c38a1e932bacccc5a2cdfb569c1972): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 2, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Mar 2, 2021

Ok, from the results it looks like we need to turn inlining off during incremental compilation, too.

@rust-log-analyzer

This comment has been minimized.

Const eval no longer runs MIR optimizations so unless this is getting
run as part of a MIR optimization like const-prop, there can be unused
type parameters even if polymorphization is enabled.
@wesleywiser wesleywiser force-pushed the enable_mir_inlining_inline_all branch from 7bf524b to d28bda0 Compare October 1, 2021 02:04
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-10 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustc_parse v0.0.0 (/checkout/compiler/rustc_parse)
   Compiling rustc_ast_lowering v0.0.0 (/checkout/compiler/rustc_ast_lowering)
   Compiling rustc_middle v0.0.0 (/checkout/compiler/rustc_middle)
   Compiling chalk-engine v0.55.0
error: internal compiler error: Encountered errors `[FulfillmentError(Obligation(predicate=Binder(TraitPredicate(<<Q as query::config::QueryConfig>::Value as std::fmt::Debug>), []), depth=0),Unimplemented)]` resolving bounds after type-checking
  = note: delayed at compiler/rustc_trait_selection/src/traits/codegen.rs:124:24


thread 'rustc' panicked at 'no errors encountered even though `delay_span_bug` issued', compiler/rustc_errors/src/lib.rs:1165:13

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.
note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md

note: rustc 1.57.0-nightly (68d4429ee 2021-10-01) running on x86_64-unknown-linux-gnu

note: compiler flags: -Z macro-backtrace -Z tls-model=initial-exec -Z unstable-options -Z binary-dep-depinfo -Z force-unstable-if-unmarked -C opt-level=3 -C embed-bitcode=no -C debuginfo=0 -C debug-assertions=on -C overflow-checks=off -C link-args=-Wl,-rpath,$ORIGIN/../lib -C prefer-dynamic -C llvm-args=-import-instr-limit=10 --crate-type lib
note: some of the compiler flags provided by cargo are hidden

query stack during panic:
end of query stack

Manishearth added a commit to Manishearth/rust that referenced this pull request Oct 1, 2021
…_message, r=michaelwoerister

Correct caller/callsite confusion in inliner message

`callee_body` is the MIR `Body` for the `callsite.callee` so this message basically says `"Inline {bar span} into bar"` when it should say `"Inline bar into foo"`.

Extracted out of rust-lang#82280
@mati865
Copy link
Contributor

mati865 commented Nov 24, 2021

AFAIK we were using old LLVM pass manager back when this wad tested. Do you think it'd be worth to retest with the new pass manager?

@wesleywiser
Copy link
Member Author

I think it definitely would be worth retesting with NewPM but a naïve rebase ICEs the compiler during bootstrap and I haven't had time to investigate further.

@bors
Copy link
Contributor

bors commented Dec 5, 2021

☔ The latest upstream changes (presumably #91475) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 5, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 2, 2022
…ll, r=oli-obk

Enable MIR inlining

Continuation of rust-lang#82280 by `@wesleywiser.`

rust-lang#82280 has shown nice compile time wins could be obtained by enabling MIR inlining.
Most of the issues in rust-lang#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:
- [x] figure out the interaction with polymorphization;
- [x] figure out how miri should deal with extern types;
- [x] silence the extra arithmetic overflow warnings;
- [x] remove the codegen fulfilment ICE;
- [x] remove the type normalization ICEs while compiling nalgebra;
- [ ] tweak the inlining threshold.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt-inlining Area: MIR inlining S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.