Const-prop bugfix: only add propagation into function call arguments #71522
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
|
CC @oli-obk |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 46d19ce8bc14982c9cf5cb64ca805f99140d7ae4 with merge 96a33e32168bc66596d898fc05314ff058465c7d... |
|
☀️ Try build successful - checks-azure |
|
Queued 96a33e32168bc66596d898fc05314ff058465c7d with parent 0612568, future comparison URL. |
|
The 7% regression looks to be entirely within llvm optimizations. There are more codegen units I think and LTO takes significantly longer. |
|
Yeah, I think our codegen partitioning logic has been pretty tightly tuned to a local maxima and as we add MIR optimizations that have bigger impacts on the end MIR, that codgen partitioning logic doesn't work as well which results in regressions (usually in incremental perf tests). I played with making a small tweak in #71349 and saw a few fairly large speedups but also a number of regressions. If we can find a good algorithm for that, I think we'll unlock a lot of optimization potential. |
|
Wait, what are codegen units and what is codegen partition? :0
Where are we on that front? What is the algorithm we're using right now, and where does it operate? Do we have a way to test it for quality? |
|
@wesleywiser there's the simplified logic you suggested. I just tested it (with some modifications because deeply nested types) and should be green. Maybe we can run a perf benchmark to see where we're at later :) |
Codegen units are the "module" of LLVM bitcode we generate so that 1) we can do code generation in parallel and 2) so incremental compilation has less to recompile when there is a change. https://rustc-dev-guide.rust-lang.org/backend/codegen.html#running-llvm-linking-and-metadata-generation Codegen partitioning is the algorithm that decides which codegen unit a function instantiation should go in. The current algorithm is here and there's a pretty good doc comment on the module: https://github.com/rust-lang/rust/blob/0862458dad90a0d80827e22e3f86e33add6d847c/src/librustc_mir/monomorphize/partitioning.rs I'm not sure we know what the "optimal" algorithm even is so I'm kind of just tinkering with it at the moment. |
Actually, would you mind applying that to #71298 so that we can test the combination of all of the changes together? |
Of course, no problem! I also found (thanks to someone in the Discord) a way to separate the two simultaneous borrows on the pass we do to remove block-local constants. The code is functionally the same; only without the |
|
Oh awesome! No rush 🙂 |
|
☔ The latest upstream changes (presumably #71200) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Wait why did github close this PR? So weird |
|
I think I broke github. I think I rebased in too hardcore a way XD |
The other testing spinoff of #71298. This one only adds the const-prop into function call arguments.
r? @wesleywiser