Online def-use analysis for copy propagation pass #76569
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
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
cc @rust-lang/wg-mir-opt |
tmiasko
force-pushed
the
online-copy-prop
branch
from
9e1d972 to
9e38b62
Compare
|
I also noticed that the previous implementation had a bug where a failure to I reorganized the changes so that this issue is fixed in separate commit (and |
|
I was planning to remove this pass entirely once my destination propagation pass lands |
|
We're not running this pass except in mir-opt-level 2 anyway, so noone is depending on it. We could just remove it entirely even without a replacement. |
|
@jonas-schievink did you have a chance to compare those two approaches, e.g., the total number of propagations made when build the standard library? (I was looking at this at some point, but the destination propagation PR fails at runtime after a rebase, and I am yet to investigate that further). |
|
No, but I've noticed that almost none of the copy-propagation tests are actually testing any propagation (they have an empty diff). I'll try to get the dest-prop PR rebased. IIRC @eddyb is pretty convinced that dest-prop is a better approach than copy-prop. |
|
Thanks, it would be nice to compare the two. For the copy propagation, based on logging that is part of this PR, I have the following stats:
The maximum is for evaluate_one_operation from gimli with 428 propagations. $ cargo new --bin empty
$ cd empty
$ env \
RUSTFLAGS=-Zmir-opt-level=2\
RUSTC_LOG=rustc_mir::transform::copy_prop=info\
cargo +stage1 -Zbuild-std build --target x86_64-unknown-linux-gnu |
|
I've rebased #72632 and fixed the assertion failures |
|
For destination propagation there are 57672 propagations when rebuilding the standard library, but there are still 21793 copy propagations afterwards. |
|
Interesting result! |
| @@ -30,177 +29,151 @@ use rustc_middle::ty::TyCtxt; | |||
| pub struct CopyPropagation; | |||
|
|
|||
| impl<'tcx> MirPass<'tcx> for CopyPropagation { | |||
| fn run_pass(&self, tcx: TyCtxt<'tcx>, _source: MirSource<'tcx>, body: &mut Body<'tcx>) { | |||
| fn run_pass(&self, tcx: TyCtxt<'tcx>, source: MirSource<'tcx>, body: &mut Body<'tcx>) { | |||
| // We only run when the MIR optimization level is > 1. | |||
| // This avoids a slow pass, and messing up debug info. | |||
| if tcx.sess.opts.debugging_opts.mir_opt_level <= 1 { | |||
There was a problem hiding this comment.
if you change this to
Suggested change
| if tcx.sess.opts.debugging_opts.mir_opt_level <= 1 { | |
| if tcx.sess.opts.debugging_opts.mir_opt_level <= 0 { |
we can perf-test this if you want
There was a problem hiding this comment.
Ok, lets try it. Later we could measure the old version for comparison.
tmiasko
force-pushed
the
online-copy-prop
branch
from
9e38b62 to
48fba1f
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 48fba1f7ed0e26b866d867e37b92cd75eca7e094 with merge 0e80e09f88ec0293bdc27db9c8cc0b283a4f75b3... |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
|
Hmm, miscompilation? |
Currently a failure to completely propagate a constant inhibits propagation of any subsequent locals. Add a test case demonstrating the problem before resolving it.
The copy propagation as currently implemented does not introduce new copy propagation opportunities. Examine each local for possible optimization only once. Additionally, this also fixes the issue where copy propagation optimization would stop without examining all locals after failing to completely propagate a constant.
The assignments that are starting point for the copy propagation are removed separately. The copy propagation does not introduce self-assignments otherwise, so the code for self-assignments elimination is functionally inert. Remove it. No functional changes intended.
Implement an online def-use analysis for the copy propagation pass. The analysis is updated in an incremental fashion as changes are made to the MIR body instead of being recomputed from scratch after each change. The analysis is intended to be equivalent to the previous one, but it also offers a new methods to: remove specific statement, remove storage markers, and replace the uses of a local with another local or a constant, all while keeping the analysis up-to date. No functional changes intended.
tmiasko
force-pushed
the
online-copy-prop
branch
from
48fba1f to
1964038
Compare
tmiasko
force-pushed
the
online-copy-prop
branch
from
1964038 to
232adee
Compare
|
There is a pre-existing issue in the copy propagation pass, I opened #76740 to track it. @jonas-schievink regarding those copy propagations performed after destination propagation, my notes show that roughly 12000 of those were arguments (they could be propagated by dest-prop if it was a little bit more flexible with regards to propagation direction, I suppose?). There were also substantial number of constant propagated as noted before. Given that copy propagation pass is working incorrectly right now and has a candidate for a replacement, I am going to close this now. |
Ah, that makes sense. I could see dest prop becoming a more general bidirectional place propagation, but it will probably take some work until that happens.
Does the existing const prop pass handle those propagations? If not, maybe that would be the right place to improve (this pass shouldn't cause any miscompilations as it already runs by default). |
Last time I looked into it, I was under the impression that constant propagation is in a large part a lint, and has to be quite conservative to avoid introducing spurious warnings, e.g., in a code that is otherwise unreachable. |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 17, 2020
…i-obk Remove the old copy propagation pass This pass was added a long time ago, and has not really seen much improvement since (apart from some great work in rust-lang#76569 that unfortunately ran into preexisting soundness issues). It is slow and unsound, and we now have a destination propagation pass that performs a related optimization and could be extended. Closes rust-lang#36673 Closes rust-lang#73717 Closes rust-lang#76740
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.
Implement an online def-use analysis for the copy propagation pass. The
analysis is updated in an incremental fashion as changes are made to the
MIR body instead of being recomputed from scratch after each change.
The analysis is intended to be functionally equivalent to the previous
one, but it also offers a new methods to: remove specific statement,
remove storage markers, and replace the uses of a local with another
local or a constant, all while keeping the analysis up-to date.
There are two functional changes to the copy propagation pass, but they
should have no effect in practice:
Each locals is examined only once as a potential destination for copy
propagation, since propagation does not introduce new optimization
opportunities.
The code for self-assignment elimination has been removed, since it
not necessary (assignments that are used for the propagation are
removed separately).
For def-use analysis changes, I would recommend reading the new version and the
old version separately, rather than examining the diff. For the copy propagation
changes, I would recommend looking at the diff that ignores whitespace during
comparison.
Closes #36673.
Closes #73717.