New pass to optimize ifconditions on integrals to switches on the integer

[simonvandel]

Fixes #75144

Pass to convert if conditions on integrals into switches on the integral.
For an example, it turns something like

_3 = Eq(move _4, const 43i32);
StorageDead(_4);
switchInt(_3) -> [false: bb2, otherwise: bb3];

into:

switchInt(_4) -> [43i32: bb3, otherwise: bb2];

[rust-highfive]

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

[simonvandel]

A further optimization would be to fold the targets of this switch into the terminator of bb0. That could be a separate PR though?

[oli-obk]

yes, please leave it to a separate PR

[oli-obk]

I'm amazed this didn't touch any other tests. Let's see what the effect of this optimization has on perf, I think it should save us from doing this dance in LLVM everywhere.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit e80c2609f5619e9b06f0f18864c60fc507578ce1 with merge 1aa291e337fc2d554250e6d7160484f473e4e131...

[bors]

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

[rust-timer]

Queued 1aa291e337fc2d554250e6d7160484f473e4e131 with parent 441fd22, future comparison URL.

[rust-timer]

Finished benchmarking try commit (1aa291e337fc2d554250e6d7160484f473e4e131): 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

[oli-obk]

looks like many smaller (< 1%) improvements. Not sure what's up with webrender-wrench-opt.incremental-unchanged, it says 1.1%, but if I click on that for details it looks like an improvement?

[simonvandel]

@oli-obk Fixed all but your comment about moving the match to a function, which I didn't understand. Can you expand on that?
Edit: I also couldn't get the to_bits to work. The function is not pub and i'm also not sure if it is really the correct function. I originally used the truncate functions to get the u128 back from a zero-extended u128. I don't know if that makes sense?
Also rebased on master to resolve the conflict.

[simonvandel]

I have fixed everything pointed out in the review, but the scalar_to_u128 function, which I don't know how to resolve, help needed.
CC @oli-obk

[tmiasko]

Make sure that previous SwitchInt used move operand before removing the comparison statement, since it might be used in other place:

pub fn f(a: i8) -> i32 {
    let b = a == 17;
    match b {
        false => 10 + b as i32,
        true => 100 + b as i32,
    }
}

[simonvandel]

Thanks, fixed this and added test for it. Interestingly enough, looking at the opt-diffs, the value being switched on is not being moved into the switch even if it is not being used later on. Is that a bug?

[oli-obk]

Yea, we may be able to fix MIR building here to use a move instead of a copy, but let's do that in a separate PR

[simonvandel]

I created issue #75993 to track fixing MIR building

[simonvandel]

Hi @oli-obk
I pushed two new commits which should resolve your comments. I can squash it down when you are done reviewing.

To resolve @tmiasko 's comment about not removing the comparison statement (Eq/Ne), I only remove it if the place of comparison result is being moved into the switchInt. That does leave the comparison undeleted in e.g. https://github.com/rust-lang/rust/pull/75370/files#diff-6577b789c4b8ff0d4a014be7009d43afR14 . Is it a problem that my current optimization actually leaves the MIR in a place where _3 is being moved twice?

If it is indeed a problem, I suggest either

1. Fix the MIR generated for an if so that _2 is moved into the switch iff _2 is not used at a later time.
2. Call a function from the simplify_locals_pass to remove dead locals

What do you think?

[simonvandel]

PR failed probably because MIR comments are being removed. I will rebase on master when @oli-obk have had time to review my latest commits

[oli-obk]

Is it a problem that my current optimization actually leaves the MIR in a place where _3 is being moved twice?

We haven't really specified the semantics yet, but I believe we'll want to enforce that you can't move things twice some time in the future. One thing you could do is to replace the moves with copies and then leave any further cleanups to later mir optimizations

[simonvandel]

One thing you could do is to replace the moves with copies and then leave any further cleanups to later mir optimizations

This is now implemented in the latest commit

[oli-obk]

Thanks! It looks like CI is failing now due to a change in our mir printing. Please rebase again and rebless the tests (for 32 bit and 64 bit both).

@bors delegate+

You can now bors r=oli-obk (with an @ in front) after you've rebased and CI passes

[bors]

✌️ @simonvandel can now approve this pull request

[simonvandel]

Rebased, squashed and CI green.
@bors r=oli-obk

[bors]

📌 Commit 10739016390affa75934f12bb876a85fa57d2500 has been approved by oli-obk

[bors]

⌛ Testing commit 10739016390affa75934f12bb876a85fa57d2500 with merge 765fa323886208e51383cdb65b45931d74ff5b11...

[bors]

💔 Test failed - checks-actions

[simonvandel]

Tests failed in the noopt tester that expected CheckedAdd in the MIR output instead of Add. I added -O as a compiler flag to the test so that it always outputs Add.
@bors r=oli-obk

[bors]

📌 Commit 23dda1b has been approved by oli-obk

[bors]

⌛ Testing commit 23dda1b with merge 286a346...

[bors]

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

[nikic]

I'm amazed this didn't touch any other tests. Let's see what the effect of this optimization has on perf, I think it should save us from doing this dance in LLVM everywhere.

FWIW, this change is actually non-canonical, and LLVM will undo it (replace the switch with an icmp+br).