New MIR optimization pass to reduce branches on match of tuples of enums

[simonvandel]

Fixes#68867 by adding a new pass that turns something like

let x:Option<()>;let y:Option<()>;match(x,y){(Some(_),Some(_)) => {0},
_ => {1}}

into something like

let x:Option<()>;let y:Option<()>;let discriminant_x = // get discriminant of x
let discriminant_y = // get discriminant of xif discriminant_x != discriminant_y {1}else{0}

The opt-diffs still have the old basic blocks like

bb3: {
_8 = discriminant((*(_4.1: &ViewportPercentageLength))); // scope 0 at $DIR/early-otherwise-branch-68867.rs:21:21: 21:30
switchInt(move _8) -> [1_isize: bb7, otherwise: bb2]; // scope 0 at $DIR/early-otherwise-branch-68867.rs:21:21: 21:30
}
bb4: {
_9 = discriminant((*(_4.1: &ViewportPercentageLength))); // scope 0 at $DIR/early-otherwise-branch-68867.rs:22:23: 22:34
switchInt(move _9) -> [2_isize: bb8, otherwise: bb2]; // scope 0 at $DIR/early-otherwise-branch-68867.rs:22:23: 22:34
}
bb5: {
_10 = discriminant((*(_4.1: &ViewportPercentageLength))); // scope 0 at $DIR/early-otherwise-branch-68867.rs:23:23: 23:34
switchInt(move _10) -> [3_isize: bb9, otherwise: bb2]; // scope 0 at $DIR/early-otherwise-branch-68867.rs:23:23: 23:34
}

These do get removed on later passes. I'm not sure if I should include those passes in the test to make it clear?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @varkor (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.

[varkor]

This looks great, but unfortunately I don't have time to review something of this size at the moment.

r? @eddyb

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit c6982e36a893aeb5647d1c7773bff287acc43a38 with merge e42444c82f1bdc51f3bd1abadcf3f7b2a9e87159...

[bors]

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

[rust-timer]

Queued e42444c82f1bdc51f3bd1abadcf3f7b2a9e87159 with parent 829d69b, future comparison URL.

[rust-timer]

Finished benchmarking try commit (e42444c82f1bdc51f3bd1abadcf3f7b2a9e87159): 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]

The pass is gated on Mir opt level 3 right now, so I don't think a perf run makes sense without removing that check. @eddyb do you want me to remove it so we can check the impact of perf?

[simonvandel]

I don't quite understand how to solve the tests failing.

- // + val: Value(Scalar(0x0000000000000002))
+ // + val: Value(Scalar(0x00000002))

Running bless locally does not change anything

[Mark-Simulacrum]

You likely need to edit the 32-bit test outputs (*.32bit) (which are not run on 64-bit machines) -- I would personally just do so manually. I wish we had a better way to support developers in doing this.

[mati865]

You also have to add // EMIT_MIR_FOR_EACH_BIT_WIDTH just like in

rust/src/test/mir-opt/combine_array_len.rs

Line 1 in d08eb98

// EMIT_MIR_FOR_EACH_BIT_WIDTH

[bors]

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

[wesleywiser]

cc @rust-lang/wg-mir-opt

[simonvandel]

Rebased on master and squashed

[oli-obk]

r? @oli-obk

[oli-obk]

instead of doing a switch and going to a block that does another switch, you can also compute another comparison:

_42 = Eq(_7,1_isize)
_43 = BitAnd(_42, _11)

and then switch on _43 to directly go to bb3

[simonvandel]

@oli-obk Hmm that works when we have only have 2 targets in the switch, but what if we have 3 targets in the switch like match (x,y,z)? See the 3_element_tuple test. Correct me if I am wrong though

[oli-obk]

That test makes bb2 dead code, and doesn't affect the match part on z at all, so it all seems to work out to me? You aren't optimizing chains longer than 2 elements with your optimization, and even if the optimization is extended to support that, we'd just end up with more of these BitAnds I think.

[simonvandel]

It's been a while, but I finally looked at it again. I don't think it is that easy to use BitAnd to avoid the extra bb. See https://github.com/rust-lang/rust/pull/75119/files#diff-37f80fe71480f2b22fb8fd1fc103fda4R88 where we need to check both 0 and 1.

[bors]

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

[oli-obk]

@bors r+

[bors]

📌 Commit 13e8e20df3307d8eaf8b6f86e73f313fcaab39b2 has been approved by oli-obk

[oli-obk]

@bors r-

[oli-obk]

@bors r+

[bors]

📌 Commit 0363694 has been approved by oli-obk

[bors]

⌛ Testing commit 0363694 with merge 2e0edc0...

[bors]

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

[ecstatic-morse]

Hi! This PR showed up in the weekly perf triage report. It showed mixed results in instruction counts. While style-servo-opt seems to have sped up, all other benchmarks run slightly slower. Was this expected?

It's possible that there's some improvement in the generated code, although I'd be a bit surprised if LLVM couldn't manage this. If there's no runtime speedup, I think we should consider disabling this by default.

[simonvandel]

Hi @ecstatic-morse
This optimization pass was created in response to #68867 which demonstrated a case where LLVM would not make this optimization.
I did a small comparison (see #68867 (comment)) which shows that the final asm that LLVM generates looks a lot cleaner. I have not benchmarked what the actualy speedup is in terms of running the function though.

[wesleywiser]

It sounds like this might be a good fit for mir-opt-level=2 then as (eventually) we want to make that the default for optimized builds while mir-opt-level=1 will remain the default for non optimized builds.

[ecstatic-morse]

@wesleywiser@simonvandel Was this ever moved to mir-opt-level=2? My personal opinion is that new optimizations should go to mir-opt-level=2 initially unless a) they result in a meaningful performance improvement for most users and b) we are very confident that they don't introduce miscompilations. I don't think this PR fits either of those criteria.

[simonvandel]

Hi @ecstatic-morse, no this pass is still running at opt-level=1. Should I create a pr to gate it at 2?