Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New mir-opt pass to simplify gotos with const values (reopening #77486) #80475

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

simonvandel
Copy link
Contributor

Reopening PR #77486

Fixes #77355

This pass optimizes the following sequence

bb2: {
    _2 = const true;
    goto -> bb3;
}

bb3: {
    switchInt(_2) -> [false: bb4, otherwise: bb5];
}

into

bb2: {
    _2 = const true;
    goto -> bb5;
}

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 29, 2020
switchInt(move _2) -> [0_isize: bb2, otherwise: bb1]; // scope 0 at $DIR/matches_reduce_branches.rs:8:22: 8:26
}

bb1: {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @oli-obk from #77486 (comment)

The MIR still contains two branches in the final MIR, so the optimization in this PR still messes this up. I'm wondering if this can be considered a bug in MatchBranchSimplification that it does not handle the _0 = const () case.

Tangentially, is _0 = const () ever needed in a function that returns unit? I think it would only make sense if _0 is considered uninitialized at the beginning of the function. But then, it would probably be a simplification to just set _0 = const () at the beginning of bb0.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_0 = const () is never needed, and I thought it's actually eliminated by InstCombine or sth. Not sure why there's still one around at this point. No ZST assignment is ever needed, you can create references to locals before they are assigned if the local is a ZST (as long as it was StorageLived I think).

@oli-obk
Copy link
Contributor

oli-obk commented Jan 1, 2021

cc @tmiasko if you have the time (and interest), could you have a look at this PR?

@ecstatic-morse
Copy link
Contributor

I'll set aside some time in the next week or so to review this. In the meantime, if you could squash your commits to remove refactorings/fixes to newly added code, it would make things a bit easier for me.

Comment on lines 76 to 78
if is_local_assigned_in_statements(place.local, &target_bb.statements) {
None?
}
Copy link
Contributor

@tmiasko tmiasko Jan 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compared to the earlier version the transform now supports switch blocks with statements. As a result it has to establish that the definition place = const reaches the switch, whereas previously it was trivially true.

Based on past experiences, I feel that doing this for an arbitrary place without a dedicated analysis will be challenging. If place is indirect it could be invalidated by other assignments, it can also contain indexing projections referencing other locals that could be modified, etc.

I would probably start with locals without any projections, and in addition to the existing check, also verify that either they never have their address taken or there are no indirect assignment in the switch block.

An example that doesn't work correctly right now:

pub fn main() {
    let mut a = &mut 0;
    if *a == 0 {
        *a = 0;
        ()
    };
    let b = &mut a;
    **b = 1;
    match *a {
        1 => {},
        _ => panic!(),
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you are right. I'll revert the newest changes so that we only support statement-less targets. Then follow ups can improve the situation

@bors
Copy link
Contributor

bors commented Jan 14, 2021

☔ The latest upstream changes (presumably #79328) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 1, 2021
@simonvandel
Copy link
Contributor Author

Fixed merge conflicts.

Hi @tmiasko, sorry for the long wait. Would you be able to do a review?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 13, 2021
@bors
Copy link
Contributor

bors commented Feb 13, 2021

⌛ Trying commit 5734754fa175f74a0081ea2823b387013c54ee10 with merge 57512cbea2265af4686716b44fd0bb86358fbcdb...

@simonvandel
Copy link
Contributor Author

@oli-obk the pass currently bails if mir opt level is less than 3 so I would expect no observable change in a perf run.If you want, I can remove the bail out so a perf run can be made.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2021

oh... oops, didn't re-check, just read your comment and started the perf run. Do you still want that perf run? If so, yea let's change it back

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2021

@bors try-

@simonvandel
Copy link
Contributor Author

@oli-obk Added a commit to disable the mir-opt-level check. Can you start the run again?

@oli-obk
Copy link
Contributor

oli-obk commented Feb 13, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@oli-obk
Copy link
Contributor

oli-obk commented Feb 23, 2021

Yea, oops, should've rechecked.

@bors r+

@bors
Copy link
Contributor

bors commented Feb 23, 2021

📌 Commit a6dccfe has been approved by oli-obk

@bors 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 Feb 23, 2021
@bors
Copy link
Contributor

bors commented Feb 23, 2021

⌛ Testing commit a6dccfe with merge 8aa2df00d8834c83aacfb232a83b3c160eeb4dae...

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bors
Copy link
Contributor

bors commented Feb 23, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 23, 2021
@bjorn3
Copy link
Member

bjorn3 commented Feb 23, 2021

@bors retry
Seems a spurious issue.

@bors 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 Feb 23, 2021
@bors
Copy link
Contributor

bors commented Feb 23, 2021

⌛ Testing commit a6dccfe with merge c1357d951bac5da7640b3cc5adac992277a89c22...

@bors
Copy link
Contributor

bors commented Feb 24, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@bjorn3
Copy link
Member

bjorn3 commented Feb 24, 2021

@bors retry

Only a single runner timed out. The rest passed.

@bors 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 Feb 24, 2021
@bors
Copy link
Contributor

bors commented Feb 24, 2021

⌛ Testing commit a6dccfe with merge 6b56603...

@bors
Copy link
Contributor

bors commented Feb 24, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6b56603 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 24, 2021
@bors bors merged commit 6b56603 into rust-lang:master Feb 24, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

if matches!(...) results in worse mir than if let ...