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

First iteration of simplify match branches #75382

Merged
merged 6 commits into from
Aug 13, 2020

Conversation

JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Aug 10, 2020

This is a simple MIR pass that attempts to convert

   bb0: {
        StorageLive(_2);
        _3 = discriminant(_1);
        switchInt(move _3) -> [0isize: bb2, otherwise: bb1];
    }

    bb1: {
        _2 = const false;
        goto -> bb3;
    }

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

into

    bb0: {
        StorageLive(_2);
        _3 = discriminant(_1);
        _2 = _3 == 0;
        goto -> bb3;
    }

There are still missing components(like checking if the assignments are bools).
Was hoping that this could get some review though.

Handles #75141

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2020
@JulianKnodt JulianKnodt force-pushed the match_branches branch 3 times, most recently from 8e45377 to c5d9787 Compare August 10, 2020 23:36
@oli-obk
Copy link
Contributor

oli-obk commented Aug 11, 2020

cc @rust-lang/wg-mir-opt

@JulianKnodt JulianKnodt force-pushed the match_branches branch 5 times, most recently from 87799bd to 7e14f9b Compare August 11, 2020 23:05
@JulianKnodt
Copy link
Contributor Author

Uh, would you happen to know why this is breaking? I can't figure out why this won't build.

@JulianKnodt JulianKnodt force-pushed the match_branches branch 2 times, most recently from 7e03e3f to 738e821 Compare August 12, 2020 01:33
@oli-obk
Copy link
Contributor

oli-obk commented Aug 12, 2020

Uh, would you happen to know why this is breaking? I can't figure out why this won't build.

your optimization breaks things. You can test with ./x.py test --stage 1 --bless src/test/mir-opt to not build rustc with your changes so you can see what's going on. Basically your optimization is too liberal in what code it is applied to, so likely we're misoptimizing some code that was never intended to get optimized

@JulianKnodt
Copy link
Contributor Author

Ah, I just assumed that since it was breaking in dependencies it was not the fault of the changes. It's good to know this may yet optimize something somewhere

@oli-obk
Copy link
Contributor

oli-obk commented Aug 12, 2020

Yea, I always work with --stage 1 because it's faster, but the additional benefit of actually getting to tests is really helpful when doing these kinds of shenanigans

@oli-obk

This comment has been minimized.

@JulianKnodt
Copy link
Contributor Author

The diff for the simple MIR test looks ok, so I'm wondering where this would break things otherwise...

@oli-obk
Copy link
Contributor

oli-obk commented Aug 13, 2020

So one thing that we overlooked is that there may be more branches pointing to the first block that we're modifying. We can work around that by not modifying it and instead cloning all statements from that block and placing them at the end of the current block

This also updates a check to ensure that this is only applied to bools
Just output the current bless'd MIR diff
The tests are still fairly broken rn
@JulianKnodt JulianKnodt marked this pull request as ready for review August 13, 2020 08:21
@oli-obk
Copy link
Contributor

oli-obk commented Aug 13, 2020

awesome!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 13, 2020

📌 Commit 46e5699 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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 13, 2020
@wesleywiser
Copy link
Member

MIR opts (hopefully 🤞) improve performance so they shouldn't be rolled up.

@bors rollup=never

@bors
Copy link
Contributor

bors commented Aug 13, 2020

⌛ Testing commit 46e5699 with merge 5e3f1b1...

@bors
Copy link
Contributor

bors commented Aug 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing 5e3f1b1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 13, 2020
@bors bors merged commit 5e3f1b1 into rust-lang:master Aug 13, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants