-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Add post-mono MIR optimizations #131650
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
base: master
Are you sure you want to change the base?
Add post-mono MIR optimizations #131650
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Add post-mono MIR passes to make mono-reachable analysis more accurate r? ghost
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (b141564): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 14.5%, secondary 1.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 22.8%, secondary 13.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.3%, secondary -2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 781.427s -> 807.023s (3.28%) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Add post-mono MIR passes to make mono-reachable analysis more accurate r? ghost
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (9233d9f): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 11.3%, secondary 2.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 10.6%, secondary 3.2%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.1%, secondary -0.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 782.104s -> 806.252s (3.09%) |
6f6737a
to
4ae3542
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Add post-mono MIR passes to make mono-reachable analysis more accurate As of rust-lang#131650 (comment) I believe most of the incr overhead comes from re-computing, re-encoding, and loading a lot more MIR when all we're actually doing is traversing through it. I think that can be addressed by caching a query that looks up the mentioned/used items for an Instance. I think the full-build regressions are pretty much just the expense of cloning, then monomorphizing, then caching the MIR.
Not sure who's good to review PRs about query system and codegen. @oli-obk ? r? oli-obk |
Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in tests/codegen/sanitizer cc @rcvalle Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
This comment has been minimized.
This comment has been minimized.
0a8e19f
to
595fdc8
Compare
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Add post-mono MIR optimizations Before this PR, all MIR passes had to operate on polymorphic MIR. Thus any MIR transform maybe unable to determine the type of an argument or local (because it's still generic) or it may be unable to determine which function a Call terminator is calling (because it's still generic). MIR transforms are a highly maintainable solution to a number of compiler problems, but this polymorphic limitation means that they are cannot solve some of our problems that we'd like them to; the most recent examples that come to mind are rust-lang#134082 which has extra limitations because of the polymorphic inliner, and rust-lang#139088 which is explicitly waiting for post-mono MIR passes to happen. In addition, the lack of post-mono MIR optimizations means that MIR optimizations just miss out on profitable optimizations, which are so valuable that we've added kludges like rust-lang#121421 (a MIR traversal that you better only run at mono-time). In addition, rustc_codegen_ssa is riddled with on-the-fly monomorphization and optimization; the logic for these trick that we do in codegen in my experience are hard to maintain, and I would much rather have those implemented in a MIR transform. So this PR adds a new query `codegen_mir` (the MIR for codegen, not that I like the name). I've then replaced _some_ of the kludges in rustc_codegen_ssa with `PostMono` variants of existing MIR transforms. I've also un-querified `check_mono_item` and put it at the end of the post-mono pass list. Those checks should be post-mono passes too, but I've tried to keep this PR to a reviewable size. It's easy to imagine lots of other places to use post-mono MIR opts and I want the usefulness of this to be clear while the diff is also manageable. --- This PR has a perf regression. I've hammered on the perf in a number of ways to get it down to what it is. incr-full builds suffer the most because they need to clone, intern, and cache a monomorphized copy of every MIR body. Things are mixed for every other build scenario. In almost all cases, binary sizes improve.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (10fa210): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowBenchmarking 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 Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 8.4%, secondary 3.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (primary 2.2%, secondary 3.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary -0.4%, secondary -0.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 776.211s -> 784.617s (1.08%) |
…tsakis Ergonomic ref counting: optimize away clones when possible This PR build on top of rust-lang/rust#134797. It optimizes codegen of ergonomic ref-counting when the type being `use`d is only known to be copy after monomorphization. We avoid codening a clone and generate bitwise copy instead. RFC: rust-lang/rfcs#3680 Tracking issue: rust-lang/rust#132290 Project goal: rust-lang/rust-project-goals#107 r? `@nikomatsakis` This PR could better sit on top of rust-lang/rust#131650 but as it did not land yet I've decided to just do minimal changes. It may be the case that doing what I'm doing regress the performance and we may need to go the full route of rust-lang/rust#131650. cc `@saethlin` in this regard.
debug!("monomorphize: self.instance={:?}", self.instance); | ||
self.instance.instantiate_mir_and_normalize_erasing_regions( | ||
self.cx.tcx(), | ||
self.cx.typing_env(), | ||
ty::EarlyBinder::bind(value), | ||
) | ||
value |
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.
Uh I guess remove this function in a follow-up? 😆
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.
Yeah. Leaving this stub in is part of me trying to keep the diff reviewable.
query build_codegen_mir(key: ty::Instance<'tcx>) -> &'tcx mir::Body<'tcx> { | ||
desc { |tcx| "finalizing codegen MIR for `{}`", tcx.def_path_str_with_args(key.def_id(), key.args) } | ||
arena_cache | ||
} |
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.
one thing you can try to improve compile times here is no_hash
. Since we are not storing it in the incremental caches anyway, we need to recompute it always as the only callers are from codegen, which either gets loaded from cache, so we don't need to call this one, or it doesn't get loaded from cache, then we can just reevaluate it. I think eval_always
would make perf worse again, as we'd rerun it even if it's not needed?
Some of the perf regressions show about twice as many |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
cdeca91
to
fd7dfb6
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
I got most of the way through implementing this before I realized that it's not this easy. One of the special powers that post-mono GVN and InstSimplify get is to always evaluate the UbChecks intrinsic and do simplifications based on that. If we run those post-mono passes as part of I'm still curious to benchmark this because maybe there's some kind of angle where we check |
Before this PR, all MIR passes had to operate on polymorphic MIR. Thus any MIR transform maybe unable to determine the type of an argument or local (because it's still generic) or it may be unable to determine which function a Call terminator is calling (because it's still generic).
MIR transforms are a highly maintainable solution to a number of compiler problems, but this polymorphic limitation means that they are cannot solve some of our problems that we'd like them to; the most recent examples that come to mind are #134082 which has extra limitations because of the polymorphic inliner, and #139088 which is explicitly waiting for post-mono MIR passes to happen.
In addition, the lack of post-mono MIR optimizations means that MIR optimizations just miss out on profitable optimizations, which are so valuable that we've added kludges like #121421 (a MIR traversal that you better only run at mono-time).
In addition, rustc_codegen_ssa is riddled with on-the-fly monomorphization and optimization; the logic for these trick that we do in codegen in my experience are hard to maintain, and I would much rather have those implemented in a MIR transform.
So this PR adds a new query
codegen_mir
(the MIR for codegen, not that I like the name). I've then replaced some of the kludges in rustc_codegen_ssa withPostMono
variants of existing MIR transforms.I've also un-querified
check_mono_item
and put it at the end of the post-mono pass list. Those checks should be post-mono passes too, but I've tried to keep this PR to a reviewable size. It's easy to imagine lots of other places to use post-mono MIR opts and I want the usefulness of this to be clear while the diff is also manageable.This PR has a perf regression. I've hammered on the perf in a number of ways to get it down to what it is. incr-full builds suffer the most because they need to clone, intern, and cache a monomorphized copy of every MIR body. Things are mixed for every other build scenario. In almost all cases, binary sizes improve.