Try MIR inlining leaf functions by default #81079
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Tomasz Miąsko <tomasz.miasko@gmail.com>
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. |
|
⌛ Trying commit 3d43b90 with merge 032277c8bdf5002b9a9d01a6a1c5db9ea04fc839... |
|
☀️ Try build successful - checks-actions |
|
Queued 032277c8bdf5002b9a9d01a6a1c5db9ea04fc839 with parent 410a546, future comparison URL. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
Finished benchmarking try commit (032277c8bdf5002b9a9d01a6a1c5db9ea04fc839): 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 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
removed
the
S-waiting-on-perf
|
r? @lcnr |
lcnr
reviewed
Jan 16, 2021
| return false; | ||
| } | ||
|
|
||
| if !did.is_local() { |
There was a problem hiding this comment.
this is unreachable. In decoding we just store false in this case 😆
The max-rss results look substantially different (worse) than the noise run, is this expected? |
oli-obk
reviewed
Jan 17, 2021
There was a problem hiding this comment.
afaict everything after f0f5e19 is not actually necessary for this PR, right?
The max-rss results look substantially different (worse) than the noise run, is this expected?
I would expect inlining to cause an increase in memory usage as MIR bodies now contain duplicates of other MIR bodies.
| @@ -204,7 +204,7 @@ macro_rules! step_identical_methods { | |||
| // In debug builds, trigger a panic on overflow. | |||
| // This should optimize completely out in release builds. | |||
| if Self::forward_checked(start, n).is_none() { | |||
| let _ = Add::add(Self::MAX, 1); | |||
| let _ = Add::add(usize::MAX, n); | |||
| @@ -1827,23 +1827,6 @@ pub fn build_session_options(matches: &getopts::Matches) -> Options { | |||
| } | |||
| Some(SymbolManglingVersion::V0) => {} | |||
| } | |||
|
|
|||
| if debugging_opts.mir_opt_level > 1 { | |||
There was a problem hiding this comment.
This commit just disables the warning, it doesn't actually disable mir inlining
nagisa
reviewed
Jan 17, 2021
| @@ -885,3 +903,48 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Integrator<'a, 'tcx> { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| pub fn is_trivial_mir(tcx: TyCtxt<'tcx>, did: DefId) -> bool { | |||
There was a problem hiding this comment.
This could really use a more semantic name. A "trivial" can mean many things, and what it means in this context is not really specified.
Given that its purpose is to check that the body is trivially inlineable, a name for your consideration: is_trivially_inlineable or perhaps is_leaf_mir.
| let mut this = Self { | ||
| increment: true, | ||
| arg_count: body.arg_count.try_into().unwrap(), | ||
| use_count: IndexVec::from_elem(0, &body.local_decls), | ||
| preserve_var_debug_info: tcx.sess.opts.optimize != OptLevel::Aggressive, |
There was a problem hiding this comment.
What criteria was used in choosing this? Other compilers generally don't try to preserve any debuginfo after -O2 which is OptLevel::Default. Futhermore, this is a MIR optimisation so it is unclear to me that deriving this from -O flag is actually what we want?
|
The unicode_normalization-check full is now at -0.4%, fixing the previous +16.4% regression. The results for packed-simd-debug & packed-simd-opt show a large regression. That is one things I would like to investigate more. The first impression is that this crate has a lot of trivial inlining opportunities while at the same time doing almost not code generation, so we are paying the cost of inlining but receive no benefits.
One additional change that follows is an extended removal of dead locals. This is something I would like to do at some point (not necessarily in the form here). The current implementation of SimplifyLocals is very conservative, and retains locals from removed dead code when they are referenced by debuginfo (e.g., it retains locals from |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on #77307 and #68828.