[mir-opt] Fix mis-optimization and other issues with the SimplifyArmIdentity pass #73949
Conversation
rust-highfive
added
the
S-waiting-on-review
|
I went through the commits each by its own, but I don't see how f00b337 can have the effect that it has (basically reverting most of the MIR opt diff from ed31211). Overall the diff lgtm, but I don't think anything beyond the debug info fixes is shown in MIR diffs. Maybe add the ui test also to MIR opt? |
If temporaries are used beyond just the temporary chain, then we can't optimize out the reads and writes.
wesleywiser
force-pushed
the
simplify_try_fixes
branch
from
9d10219 to
e16d6a6
Compare
|
@oli-obk I think I forgot to re-bless the mir-opt tests after splitting this into different commits so that's probably part of the issue. I've implemented your suggestion and blessed each commit so you can see the changes. |
| @@ -397,7 +442,9 @@ impl LocalUseCounter { | |||
|
|
|||
| impl<'tcx> Visitor<'tcx> for LocalUseCounter { | |||
| fn visit_local(&mut self, local: &Local, context: PlaceContext, _location: Location) { | |||
| if context.is_storage_marker() { | |||
| if context.is_storage_marker() | |||
| || context == PlaceContext::NonUse(NonUseContext::VarDebugInfo) | |||
There was a problem hiding this comment.
are there any NonUseContext that are relevant here? The only one left that isn't covered is AscribeUserTy. So this could just be if let PlaceContext::NonUse(_) = context` I think.
There was a problem hiding this comment.
Hmm I guess those are unused after NLL runs right? Maybe we should have a separate pass that runs at the beginning of the optimization phase and removes all of those?
There was a problem hiding this comment.
true, and then changing this site to the general match on NonUseContext doesn't actually change anything, anymore, so let's do that in a separate PR
| _3 = move ((_1 as Some).0: std::boxed::Box<()>); // scope 0 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:14: 4:15 | ||
| ((_0 as Some).0: std::boxed::Box<()>) = move _3; // scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:20: 4:27 | ||
| discriminant(_0) = 1; // scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:20: 4:27 | ||
| _0 = move _1; // scope 1 at $DIR/simplify-locals-removes-unused-discriminant-reads.rs:4:20: 4:27 |
There was a problem hiding this comment.
oh now I realize how this is happening... we were missing optimizations because we were marking them as used because of debuginfo
Thanks! This is absolutely great. I have one more nit, r=me with that + reblessing aftwards. |
|
@bors r+ |
|
📌 Commit e16d6a6 has been approved by |
|
🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened |
bors
added
S-waiting-on-bors
|
Since this messes with mir-opt stuff, I'm going to say @bors rollup=never Just in case 🙂 |
|
@bors p=1 |
|
Is the rollup=never for perf stuff or just because you think this will fail a rollup? Because I'm game for including this in a rollup, I typically include at least one iffy PR in a rollup -- if it fails we get early feedback on the PR. |
|
@Manishearth More out of an abundance of caution although, since this pass is disabled, it shouldn't have any effect (performance or correctness). |
|
I'm gonna un-never it then, rollups take this kind of risk pretty typically 😄 @bors r- rollup=maybe |
bors
added
S-waiting-on-author
|
@bors r=oli-obk p=0 oops |
|
📌 Commit e16d6a6 has been approved by |
bors
added
S-waiting-on-bors
|
Sounds good. Thanks @Manishearth! |
…arth Rollup of 12 pull requests Successful merges: - rust-lang#73140 (Fallback to xml.etree.ElementTree) - rust-lang#73670 (Add `format_args_capture` feature) - rust-lang#73693 (Use exhaustive match in const_prop.rs) - rust-lang#73845 (Use &raw in A|Rc::as_ptr) - rust-lang#73861 (Create E0768) - rust-lang#73881 (Standardize bibliographic citations in rustc API docs) - rust-lang#73925 (Improve comments from rust-lang#72617, as suggested by RalfJung) - rust-lang#73949 ([mir-opt] Fix mis-optimization and other issues with the SimplifyArmIdentity pass) - rust-lang#73984 (Edit docs for rustc_data_structures::graph::scc) - rust-lang#73985 (Fix "getting started" link) - rust-lang#73997 (fix typo) - rust-lang#73999 (Bump mingw-check CI image from Ubuntu 16.04 to 18.04.) Failed merges: - rust-lang#74000 (add `lazy_normalization_consts` feature gate) r? @ghost
This does not yet attempt re-enabling the pass, but it does resolve a number of issues with the pass.
r? @oli-obk
I believe this closes #73223.