[mir-opt] Fix mis-optimization and other issues with the SimplifyArmIdentity pass#73949

wesleywiser · 2020-07-02T02:59:03Z

This does not yet attempt re-enabling the pass, but it does resolve a number of issues with the pass.

I believe this closes #73223.

oli-obk · 2020-07-02T07:24:21Z

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 · 2020-07-03T01:21:01Z

@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.

oli-obk · 2020-07-03T10:30:08Z

src/librustc_mir/transform/simplify_try.rs

    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)


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.

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?

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

oli-obk · 2020-07-03T10:31:01Z

...ir-opt/simplify-locals-removes-unused-discriminant-reads/32bit/rustc.map.SimplifyLocals.diff

-          _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


oh now I realize how this is happening... we were missing optimizations because we were marking them as used because of debuginfo

oli-obk · 2020-07-03T10:34:15Z

I've implemented your suggestion and blessed each commit so you can see the changes.

Thanks! This is absolutely great. I have one more nit, r=me with that + reblessing aftwards.

oli-obk · 2020-07-03T12:35:26Z

@bors r+

bors · 2020-07-03T12:35:27Z

📌 Commit e16d6a6 has been approved by oli-obk

bors · 2020-07-03T12:35:29Z

🌲 The tree is currently closed for pull requests below priority 100, this pull request will be tested once the tree is reopened

wesleywiser · 2020-07-03T14:37:23Z

Since this messes with mir-opt stuff, I'm going to say

@bors rollup=never

Just in case 🙂

Manishearth · 2020-07-03T14:54:10Z

@bors p=1

Manishearth · 2020-07-03T15:07:26Z

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.

wesleywiser · 2020-07-03T15:23:30Z

@Manishearth More out of an abundance of caution although, since this pass is disabled, it shouldn't have any effect (performance or correctness).

Manishearth · 2020-07-03T15:30:13Z

I'm gonna un-never it then, rollups take this kind of risk pretty typically 😄

@bors r- rollup=maybe

Manishearth · 2020-07-03T15:30:30Z

@bors r=oli-obk p=0

oops

bors · 2020-07-03T15:30:31Z

📌 Commit e16d6a6 has been approved by oli-obk

wesleywiser · 2020-07-03T15:33:19Z

Sounds good. Thanks @Manishearth!

@ghost

…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

rust-highfive assigned oli-obk Jul 2, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2020

wesleywiser mentioned this pull request Jul 2, 2020

Slice index becomes wrong (beta regression) #73223

Closed

wesleywiser added 4 commits July 2, 2020 20:54

Add test for simplify-try misoptimization

e3f599c

[mir-opt] Prevent mis-optimization when SimplifyArmIdentity runs

9248d90

If temporaries are used beyond just the temporary chain, then we can't optimize out the reads and writes.

Fix debuginfo so that it points to the correct local

24bfdc9

Fix missing return in optimization_applies()

e16d6a6

wesleywiser force-pushed the simplify_try_fixes branch from 9d10219 to e16d6a6 Compare July 3, 2020 01:16

oli-obk reviewed Jul 3, 2020

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 3, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 3, 2020

Manishearth mentioned this pull request Jul 4, 2020

Rollup of 12 pull requests #74019

Merged

bors merged commit 60cad20 into rust-lang:master Jul 4, 2020

cuviper added this to the 1.46 milestone May 2, 2024

Uh oh!

Conversation

wesleywiser commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oli-obk commented Jul 2, 2020

Uh oh!

wesleywiser commented Jul 3, 2020

Uh oh!

oli-obk Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

wesleywiser Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

oli-obk Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

oli-obk Jul 3, 2020

Choose a reason for hiding this comment

Uh oh!

oli-obk commented Jul 3, 2020

Uh oh!

oli-obk commented Jul 3, 2020

Uh oh!

bors commented Jul 3, 2020

Uh oh!

bors commented Jul 3, 2020

Uh oh!

wesleywiser commented Jul 3, 2020

Uh oh!

Manishearth commented Jul 3, 2020

Uh oh!

Manishearth commented Jul 3, 2020

Uh oh!

wesleywiser commented Jul 3, 2020

Uh oh!

Manishearth commented Jul 3, 2020

Uh oh!

Manishearth commented Jul 3, 2020

Uh oh!

bors commented Jul 3, 2020

Uh oh!

wesleywiser commented Jul 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wesleywiser commented Jul 2, 2020 •

edited

Loading