-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Remove the old copy propagation pass #77373
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
You'll probably know better than me who a good reviewer here is - I don't have sufficient context I think, so I'll let you reassign. |
r? @oli-obk |
@@ -472,7 +471,6 @@ fn run_optimization_passes<'tcx>( | |||
&simplify_try::SimplifyArmIdentity, | |||
&simplify_try::SimplifyBranchSame, | |||
&dest_prop::DestinationPropagation, | |||
©_prop::CopyPropagation, | |||
&simplify_branches::SimplifyBranches::new("after-copy-prop"), |
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.
this should probably be renamed
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.
Renamed to "final"
@@ -1,39 +0,0 @@ | |||
// Check that CopyPropagation does not propagate an assignment to a function argument |
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.
Do we have tests for all of these situations for the dest-prop opt?
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.
I ported this test over, the results look correct
lgtm. r=me with nits resolved |
@bors try @rust-timer queue |
Awaiting bors try build completion |
⌛ Trying commit 8aa1239de1c8daab86d44fdd93b5ed1f91369136 with merge f60f56e913fc86e229f05dff68d2853916dbf706... |
☀️ Try build successful - checks-actions, checks-azure |
Queued f60f56e913fc86e229f05dff68d2853916dbf706 with parent 3bbc443, future comparison URL. |
Finished benchmarking try commit (f60f56e913fc86e229f05dff68d2853916dbf706): 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 |
@bors rollup- |
(the pass doesn't run by default, so there's no perf impact) |
@jonas-schievink Could you resolve the merge conflicts? |
@bors r+ |
📌 Commit dc31777 has been approved by |
☀️ Test successful - checks-actions, checks-azure |
This pass was added a long time ago, and has not really seen much improvement since (apart from some great work in #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 #36673
Closes #73717
Closes #76740