MIR-OPT: Make SimplifyBranchSame able to remove identity match with fieldless variant #74748

simonvandel · 2020-07-25T15:59:05Z

Modifies SimplifyBranchSame so that it can see that the statements can be considered equal in the following example
_0 = _1 and discriminant(_0) = discriminant(0) are considered equal if 0 is a fieldless variant of an enum.

rust-highfive · 2020-07-25T15:59:09Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

src/librustc_mir/transform/simplify_try.rs

src/test/mir-opt/simplify-arm/rustc.id.SimplifyDiscriminantArm.diff

simonvandel · 2020-07-25T22:51:32Z

src/test/mir-opt/simplify-arm.rs

@@ -1,9 +1,12 @@
-// compile-flags: -Z mir-opt-level=1
+// compile-flags: -Z mir-opt-level=2


I changed this to 2 since that is the level that SimplifyArmIdentity runs at

simonvandel · 2020-07-26T07:44:03Z

I wonder if converting a SetDiscriminant to an assignment is actually a pessimisation if SimplifyBranchSame for some reason cannot unify the branches. If the size difference of enum variants are sufficiently big, it seems that it could be an issue.

One way to deal with this would be to build the checks for the SimplifyDiscriminantArm pass directly into SimplifyBranchSame, allowing it to simply union the branches of the conditions meet, such that SetDiscriminant is the same behavior as an assignment. Then SimplifyDiscriminantArm would not exist and would therefore not be able to create a pessimisation.

Any thoughts?

petrochenkov · 2020-07-26T09:51:48Z

r? @oli-obk

oli-obk · 2020-07-26T11:13:09Z

One way to deal with this would be to build the checks for the SimplifyDiscriminantArm pass directly into SimplifyBranchSame, allowing it to simply union the branches of the conditions meet, such that SetDiscriminant is the same behavior as an assignment. Then SimplifyDiscriminantArm would not exist and would therefore not be able to create a pessimisation.

Yea, I think making the SimplifyDiscriminantArm optimization smarter would be a better idea, there are many issues with "optimizing" SetDiscriminant (in https://github.com/rust-lang/rust/pull/73656/files, too). Basically we need to support zero fields in addition to the current support for N fields for N > 0.

simonvandel · 2020-07-26T20:32:15Z

@oli-obk

making the SimplifyDiscriminantArm optimization smarter would be a better idea

Do you mean making the SimplifyBranchSame optimization smarter would be a better idea?

oli-obk · 2020-07-27T11:36:00Z

Do you mean making the SimplifyBranchSame optimization smarter would be a better idea?

oops yea.

wesleywiser · 2020-07-28T12:23:39Z

I agree, I think we should work on improving the SimplifyBranchSame pass instead of introducing another pass. SimplifyBranchSame already checks for a number of required preconditions (such as ensuring we don't introduce a use-after-move) that would have to be replicated in this pass anyway.

bors · 2020-07-29T19:52:48Z

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

simonvandel · 2020-08-06T19:20:05Z

Hi @wesleywiser and @oli-obk
I got around to doing the optimization inside of SimplifyBranchSame now. Do you want to review it?

simonvandel · 2020-08-06T20:23:55Z

So i'm a bit unsure of what's going on in the failed tests. "set descriminant on adt that is not enum" It arrives here:
https://github.com/rust-lang/rust/blob/ff991c40a93d709b529d9b6e4b5cdb43f44c7f88/src/librustc_mir/transform/simplify_try.rs#L679-L682
where the place is from a SetDescriminant found here:
https://github.com/rust-lang/rust/blob/ff991c40a93d709b529d9b6e4b5cdb43f44c7f88/src/librustc_mir/transform/simplify_try.rs#L711-L722

error: internal compiler error: src/librustc_mir/transform/simplify_try.rs:679:22: set descriminant on adt that is not enum. Place: (*(_1.0: &mut [static generator@/home/simon/Documents/rust/src/test/ui/async-await/async-await.rs:197:24: 199:14 x:u8 {std::future::ResumeTy, u8, impl std::future::Future, ()}])), place_type: [static generator@/home/simon/Documents/rust/src/test/ui/async-await/async-await.rs:197:24: 199:14 x:u8 {std::future::ResumeTy, u8, impl std::future::Future, ()}]

Is my assumption that SetDescriminant only ever has a place of a enum wrong? The above error message seems like the place is a generator

bjorn3 · 2020-08-06T21:07:46Z

Generators also use SetDiscriminant.

src/librustc_mir/transform/simplify_try.rs

wesleywiser

Can you rebase and squash as well? Thanks!

src/librustc_mir/transform/simplify_try.rs

simonvandel · 2020-08-10T19:18:10Z

Hi @wesleywiser
Squashed. It would be interesting to see if this does anything to compile times. Can you schedule a perf check?

wesleywiser · 2020-08-10T19:31:56Z

Sure thing! As I recall, there were some slight wins the last time we enabled this so hopefully we see those again (and larger)!

@bors try @rust-timer queue

rust-timer · 2020-08-10T19:31:57Z

Awaiting bors try build completion

bors · 2020-08-10T19:32:08Z

⌛ Trying commit 755d6efa3faa906f1fb6424c390bfc3ce8a2ff3b with merge 9b2742608f990cfe089efec9466a9ca39dce0501...

bors · 2020-08-10T20:19:02Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 9b2742608f990cfe089efec9466a9ca39dce0501 (9b2742608f990cfe089efec9466a9ca39dce0501)

rust-timer · 2020-08-10T20:19:04Z

Queued 9b2742608f990cfe089efec9466a9ca39dce0501 with parent 1275cc1, future comparison URL.

rust-timer · 2020-08-10T22:56:42Z

Finished benchmarking try commit (9b2742608f990cfe089efec9466a9ca39dce0501): 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 rollup- to bors.

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

simonvandel · 2020-08-11T05:14:37Z

Perf looks pretty neutral. But i think it’s because SimplifyArmIdentity is not running on mir opt level 1. If it was, a lot more branches would match the optimization. @wesleywiser should we land the current PR? And then I’m wondering if we can lower the opt level for SimplifyArmIdentity.

wesleywiser · 2020-08-11T14:43:26Z

src/test/mir-opt/simplify_arm.id_try.SimplifyArmIdentity.diff

          scope 5 {
          }
      }
+      scope 6 {
+          debug self => _4;                // in scope 6 at $SRC_DIR/core/src/result.rs:LL:COL


It's very strange that this is being added (debug self => doesn't' appear at all on the left side)

EDIT: This is probably because we switched -Zmir-opt-level=2 on which means the inliner is running.

src/librustc_mir/transform/simplify_try.rs

wesleywiser · 2020-08-11T15:00:28Z

But i think it’s because SimplifyArmIdentity is not running on mir opt level 1.

Yeah, you're right!

@wesleywiser should we land the current PR?

It looks good to me, @oli-obk do you want to review as well?

And then I’m wondering if we can lower the opt level for SimplifyArmIdentity.

We had to increase the opt-level last time because of regressions found during the beta period so I was planning on trying again after the next beta branch in about two weeks time. That way we maximize the time it has on the nightly channel and we can hopefully catch and fix any regressions before it reaches beta.

oli-obk · 2020-08-11T15:15:21Z

do you want to review as well?

the MIR diffs lgtm, I didn't look at the impl. If you want I can try to find some time for it, but imo you can merge at your discretion

simonvandel · 2020-08-11T19:28:25Z

Pushed @wesleywiser 's suggestion about or-patterns

simonvandel · 2020-08-11T20:48:28Z

@wesleywiser The 32 bit tests are being annoying again. Would it be feasible to have --bless somehow also update 32bit tests? I always forget to do that

wesleywiser · 2020-08-12T00:40:01Z

@simonvandel You can do that by telling x.py to build and run a 32-bit target. For example: ./x.py test --stage 1 -i --target i686-unknown-linux-gnu src/test/mir-opt/

wesleywiser · 2020-08-13T11:18:56Z

This just needs the 32-bit tests blessed and then we can merge it. @simonvandel if you need help with that, I'd be happy to pull your branch locally and bless the tests.

simonvandel · 2020-08-13T12:45:38Z

@wesleywiser I won't be able to do it in the next couple of days, so feel free to drive it onwards.

…= _1` and `discriminant(_0) = discriminant(0)` are considered equal if 0 is a fieldless variant of an enum

simonvandel · 2020-08-16T20:10:12Z

Hi @wesleywiser
I pushed the blessing of 32bit now

wesleywiser · 2020-08-17T16:48:20Z

Thanks @simonvandel!

Since this is a mir-opt, we should avoid rolling up in case there is a perf impact or we need to do a bisection.

@bors r+ rollup=never

bors · 2020-08-17T16:48:21Z

📌 Commit 293756c has been approved by wesleywiser

bors · 2020-08-17T18:33:29Z

⌛ Testing commit 293756c with merge 33c96b4...

bors · 2020-08-17T20:51:52Z

☀️ Test successful - checks-actions, checks-azure
Approved by: wesleywiser
Pushing 33c96b4 to master...

rust-highfive assigned petrochenkov Jul 25, 2020

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

simonvandel commented Jul 25, 2020

View reviewed changes

src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved

bjorn3 reviewed Jul 25, 2020

View reviewed changes

src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved

simonvandel commented Jul 25, 2020

View reviewed changes

src/test/mir-opt/simplify-arm/rustc.id.SimplifyDiscriminantArm.diff Outdated Show resolved Hide resolved

simonvandel marked this pull request as ready for review July 25, 2020 22:48

simonvandel commented Jul 25, 2020

View reviewed changes

rust-highfive assigned oli-obk and unassigned petrochenkov Jul 26, 2020

simonvandel force-pushed the simplify-discriminant-arm branch from 4b92f89 to f362a0b Compare August 6, 2020 19:13

simonvandel changed the title ~~MIR-OPT: New pass to allow removal of identity match with fieldless variant~~ MIR-OPT: Make SimplifyBranchSame able to remove identity match with fieldless variant Aug 6, 2020

wesleywiser reviewed Aug 8, 2020

View reviewed changes

src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved

src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved

wesleywiser reviewed Aug 10, 2020

View reviewed changes

src/librustc_mir/transform/simplify_try.rs Outdated Show resolved Hide resolved

simonvandel force-pushed the simplify-discriminant-arm branch from e1a95b4 to 755d6ef Compare August 10, 2020 19:14

wesleywiser reviewed Aug 11, 2020

View reviewed changes

simonvandel force-pushed the simplify-discriminant-arm branch from 755d6ef to d29da9b Compare August 11, 2020 19:27

simonvandel force-pushed the simplify-discriminant-arm branch from d29da9b to d2fc8b0 Compare August 11, 2020 20:13

wesleywiser approved these changes Aug 13, 2020

View reviewed changes

Implement 'considered equal' for statements, so that for example `_0 …

293756c

…= _1` and `discriminant(_0) = discriminant(0)` are considered equal if 0 is a fieldless variant of an enum

simonvandel force-pushed the simplify-discriminant-arm branch from d2fc8b0 to 293756c Compare August 16, 2020 20:06

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 17, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 17, 2020

bors merged commit 33c96b4 into rust-lang:master Aug 17, 2020

This was referenced Aug 17, 2020

diagnostics: shorten paths of unique symbols #73996

Merged

Suppress verbose MIR comments for trivial types #75566

Merged

oli-obk mentioned this pull request Sep 17, 2020

starting from nightly-2020-08-18 rustls can't connect to some websites #76803

Closed

cuviper added this to the 1.47.0 milestone May 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR-OPT: Make SimplifyBranchSame able to remove identity match with fieldless variant #74748

MIR-OPT: Make SimplifyBranchSame able to remove identity match with fieldless variant #74748

simonvandel commented Jul 25, 2020 •

edited

Loading

rust-highfive commented Jul 25, 2020

simonvandel Jul 25, 2020 •

edited

Loading

simonvandel commented Jul 26, 2020

petrochenkov commented Jul 26, 2020

oli-obk commented Jul 26, 2020

simonvandel commented Jul 26, 2020

oli-obk commented Jul 27, 2020

wesleywiser commented Jul 28, 2020

bors commented Jul 29, 2020

simonvandel commented Aug 6, 2020

simonvandel commented Aug 6, 2020

bjorn3 commented Aug 6, 2020

wesleywiser left a comment

simonvandel commented Aug 10, 2020

wesleywiser commented Aug 10, 2020

rust-timer commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

rust-timer commented Aug 10, 2020

rust-timer commented Aug 10, 2020

simonvandel commented Aug 11, 2020

wesleywiser Aug 11, 2020

wesleywiser commented Aug 11, 2020

oli-obk commented Aug 11, 2020

simonvandel commented Aug 11, 2020

simonvandel commented Aug 11, 2020

wesleywiser commented Aug 12, 2020

wesleywiser commented Aug 13, 2020

simonvandel commented Aug 13, 2020 •

edited

Loading

simonvandel commented Aug 16, 2020

wesleywiser commented Aug 17, 2020

bors commented Aug 17, 2020

bors commented Aug 17, 2020

bors commented Aug 17, 2020

		@@ -1,9 +1,12 @@
		// compile-flags: -Z mir-opt-level=1
		// compile-flags: -Z mir-opt-level=2

MIR-OPT: Make SimplifyBranchSame able to remove identity match with fieldless variant #74748

MIR-OPT: Make SimplifyBranchSame able to remove identity match with fieldless variant #74748

Conversation

simonvandel commented Jul 25, 2020 • edited Loading

rust-highfive commented Jul 25, 2020

simonvandel Jul 25, 2020 • edited Loading

Choose a reason for hiding this comment

simonvandel commented Jul 26, 2020

petrochenkov commented Jul 26, 2020

oli-obk commented Jul 26, 2020

simonvandel commented Jul 26, 2020

oli-obk commented Jul 27, 2020

wesleywiser commented Jul 28, 2020

bors commented Jul 29, 2020

simonvandel commented Aug 6, 2020

simonvandel commented Aug 6, 2020

bjorn3 commented Aug 6, 2020

wesleywiser left a comment

Choose a reason for hiding this comment

simonvandel commented Aug 10, 2020

wesleywiser commented Aug 10, 2020

rust-timer commented Aug 10, 2020

bors commented Aug 10, 2020

bors commented Aug 10, 2020

rust-timer commented Aug 10, 2020

rust-timer commented Aug 10, 2020

simonvandel commented Aug 11, 2020

wesleywiser Aug 11, 2020

Choose a reason for hiding this comment

wesleywiser commented Aug 11, 2020

oli-obk commented Aug 11, 2020

simonvandel commented Aug 11, 2020

simonvandel commented Aug 11, 2020

wesleywiser commented Aug 12, 2020

wesleywiser commented Aug 13, 2020

simonvandel commented Aug 13, 2020 • edited Loading

simonvandel commented Aug 16, 2020

wesleywiser commented Aug 17, 2020

bors commented Aug 17, 2020

bors commented Aug 17, 2020

bors commented Aug 17, 2020

simonvandel commented Jul 25, 2020 •

edited

Loading

simonvandel Jul 25, 2020 •

edited

Loading

simonvandel commented Aug 13, 2020 •

edited

Loading