MIR opt: separate constant predecessors of a switch #85646

Moxinilian · 2021-05-24T21:37:22Z

For each block S ending with a switch, this pass copies S for each of S's predecessors that seem to assign the value being switched over as a const. This is done using a somewhat simple heuristic to determine what seems to be a const transitively.

More precisely, this is what the pass does:

find a block that ends in a switch
track if there is an unique place set before the current basic block that determines the result of the switch (this is the part that resolves switching over discriminants)
if there is, iterate over the parents that have a reasonable terminator and find if the found determining place is likely to be (transitively) set from a const within that parent block
if so, add the corresponding edge to a vector of edges to duplicate
once this is done, iterate over the found edges: copy the target block and replace the reference to the target block in the origin block with the new block

This pass is not optimal and could probably duplicate in more cases, but the intention was mostly to address cases like in #85133 or #85365, to avoid creating new enums that get destroyed immediately afterwards (notably making the new try v2 ? desugar zero-cost).

A benefit of this pass working the way it does is that it is easy to ensure its correctness: the worst that can happen is for it to needlessly copy a basic block, which is likely to be destroyed by cleanup passes afterwards. The complex parts where aliasing matters are only heuristics and the hard work is left to further passes like ConstProp.

LLVM blocker

Unfortunately, I believe it would be unwise to enable this optimization by default for now. Indeed, currently switch lowering passes like SimplifyCFG in LLVM lose the information on the set of possible variant values, which means it tends to actually generate worse code with this optimization enabled. A fix would have to be done in LLVM itself. This is something I also want to look into. I have opened a bug report at the LLVM bug tracker.

When this is done, I hope we can enable this pass by default. It should be fairly fast and I think it is beneficial in many cases. Notably, it should be a sound alternative to simplify-arm-identity. By the way, ConstProp only seems to pick up the optimization in functions that are not generic. This is however most likely an issue in ConstProp that I will look into afterwards.

This is my first contribution to rustc, and I would like to thank everyone on the Zulip mir-opt chat for the help and support, and especially @scottmcm for the guidance.

rust-highfive · 2021-05-24T21:37:25Z

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.

cjgillot

Thanks for this optimization. We already saw on zulip it's potential. I left some nits.
Please also rebase on master instead of merging.

compiler/rustc_mir/src/transform/mod.rs

compiler/rustc_mir/src/transform/separate_const_switch.rs

cjgillot · 2021-05-24T22:03:44Z

compiler/rustc_mir/src/transform/separate_const_switch.rs

+    }
+
+    let blocks = body.basic_blocks_mut();
+    for (pred_id, target_id) in new_edges {


Can the two loops be merged?

The reason I separated the loops is that according to somebody on Zulip, mutating the blocks vector invalidates the predecessor cache which is apparently expensive to re-compute. So this way I can have an immutable loop with a stable copy of the predecessors without having to clone it. I don't know if it's worth it. The other approach would be to clone the predecessors beforehand so we don't use the recomputed one.

I could not come up with a good way to merge the loops. If you have a suggestion, I'd be happy to reconsider.

src/test/mir-opt/separate_const_switch.rs

Moxinilian · 2021-05-25T21:50:44Z

New developments revealed that:

The simplification is actually not necessary, but ConstProp seems to not like generic functions.
The LLVM blocker is not actually what I thought and can probably be worked around from rustc. edit: never mind

More information in updated OP.

Moxinilian · 2021-05-25T22:16:13Z

After destroying my branch a couple times trying solutions found online, I can assert with a great amount of certainty that I have no idea how to rebase on master in the current state of things. Could somebody guide me through it?

compiler/rustc_mir/src/transform/separate_const_switch.rs

varkor · 2021-05-27T21:54:53Z

r? @cjgillot seems to have this under control :)

nagisa · 2021-06-06T15:30:20Z

After destroying my branch a couple times trying solutions found online, I can assert with a great amount of certainty that I have no idea how to rebase on master in the current state of things. Could somebody guide me through it?

Try this

git fetch origin

git fetch origin refs/pull/85646/head
# From https://github.com/rust-lang/rust
#  * branch                    refs/pull/85646/head -> FETCH_HEAD

git checkout -f FETCH_HEAD
# Updating files: 100% (1091/1091), done.
# Previous HEAD position was 55a7f62aa88 Do not allow JSON targets to set is-builtin: true
# HEAD is now at d7ccc641cde addressed some reviews

git rebase -i origin/master
# Opens an editor, exit the editor without any changes the commit order or the operations
# Once the editor exits the rebase seems to go through fine

git checkout -b separate-const-switch-rebased

git push fork separate-const-switch-rebased:refs/heads/separate-const-switch -f
#        ^--^
# You may need to change the "fork" to whatever your fork remote is named.

which resulted in this (in your case it should update this PR).

If this end ups failing you should be able to return to your original state by checking out your original branch/commit (possibly after aborting the rebase first, depending on when things go wrong)

Moxinilian · 2021-06-09T18:30:57Z

Thank you for the answer @nagisa.
Unfortunately when I try your instructions, the rebase hangs with a conflict on /Cargo.toml. If I try to resolve it by accepting the incoming change then running git add Cargo.toml, when running git rebase --continue, the command line reports:

You must edit all merge conflicts and then
mark them as resolved using git add

which makes no sense to me as I just did it. Do you perhaps know what is going on?

@cjgillot I tried the instructions you sent to me in private to different but similar results. Notably, after doing what you suggested (to simply rebase using git rebase upstream/master separate-const-switch, the rebase process complains with a cryptic

error: add_cacheinfo failed to refresh for path 'src/tools/rls'; merge aborting.
hint: Could not execute the todo command
hint:
hint: pick bf8cf362f60b978b9f52df1100dcec51560e6bec Squashed commit of the following:
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint: git rebase --edit-todo
hint: git rebase --continue
Could not apply bf8cf362f60... Squashed commit of the following:

(this is the end of the report)

edit: I think I'll just copy my changes to a new branch and force push that

Moxinilian · 2021-06-09T22:39:36Z

I think it is ready for further review now. Once you are happy with its state, would it make sense to run some performance tests to know its impact on build time and runtime performance?

Cargo.lock

lqd · 2021-06-10T17:09:02Z

@bors try @rust-timer queue

rust-timer · 2021-06-10T17:09:03Z

Awaiting bors try build completion.

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

bors · 2021-06-10T17:09:12Z

⌛ Trying commit 8b1094be858f3079777566ab0e0ad25405de6362 with merge 54784f9434beb0631f268648b9c03ce81a2116d1...

compiler/rustc_mir/src/transform/separate_const_switch.rs

bors · 2021-06-10T18:09:38Z

☀️ Try build successful - checks-actions
Build commit: 54784f9434beb0631f268648b9c03ce81a2116d1 (54784f9434beb0631f268648b9c03ce81a2116d1)

rust-timer · 2021-06-10T18:09:40Z

Queued 54784f9434beb0631f268648b9c03ce81a2116d1 with parent 40c1623, future comparison URL.

rust-timer · 2021-06-10T21:10:55Z

Finished benchmarking try commit (54784f9434beb0631f268648b9c03ce81a2116d1): 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
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

Moxinilian · 2021-06-20T10:26:16Z

As discussed on Zulip, I decided to keep this pass simple for now, so it ignores projections. I also improved documentation.
I would still like to handle projections later on, it's possible to do something better but it's a bit more complex, but for now I think it's already good to have this merged and look at the LLVM issue.

nagisa · 2021-06-20T15:03:22Z

If we proceed with this, I think it is important to decide what a canonically optimized MIR looks like, so that all the optimizations can work towards the same goal. This specific optimization sounds a lot like something a simplifycfg-like pass could undo right after this transformation is done.

Moxinilian · 2021-06-20T16:23:35Z

That's true. I think there is something about size vs speed to consider here. Clearly this pass gives up on generated code size in favor of (theoretical) performance. How would we translate the will of the user with respect to this with the current infrastructure?

un-update itertools improve predecessor amount short-circuiting cleanup and comments somewhat improved drawing

compiler/rustc_mir/src/transform/separate_const_switch.rs

cjgillot · 2021-07-25T09:21:03Z

@bors r+

bors · 2021-07-25T09:21:05Z

📌 Commit a77e2ad has been approved by cjgillot

bors · 2021-07-25T13:51:53Z

⌛ Testing commit a77e2ad with merge 70f7471...

bors · 2021-07-25T16:17:52Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 70f7471 to master...

rust-highfive assigned varkor May 24, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 24, 2021

cjgillot reviewed May 24, 2021

View reviewed changes

cjgillot self-assigned this May 25, 2021

Moxinilian force-pushed the separate-const-switch branch 2 times, most recently from bf8cf36 to d7ccc64 Compare May 25, 2021 22:12

cjgillot reviewed May 27, 2021

View reviewed changes

compiler/rustc_mir/src/transform/separate_const_switch.rs Outdated Show resolved Hide resolved

cjgillot reviewed May 27, 2021

View reviewed changes

compiler/rustc_mir/src/transform/separate_const_switch.rs Outdated Show resolved Hide resolved

cjgillot reviewed May 27, 2021

View reviewed changes

compiler/rustc_mir/src/transform/separate_const_switch.rs Outdated Show resolved Hide resolved

cjgillot reviewed May 27, 2021

View reviewed changes

compiler/rustc_mir/src/transform/separate_const_switch.rs Outdated Show resolved Hide resolved

rust-highfive unassigned varkor May 27, 2021

Moxinilian force-pushed the separate-const-switch branch 2 times, most recently from b1faed9 to e857e11 Compare June 9, 2021 22:37

scottmcm reviewed Jun 9, 2021

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

Moxinilian force-pushed the separate-const-switch branch from 587193e to 2f3662c Compare June 10, 2021 16:57

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 10, 2021

cjgillot reviewed Jun 10, 2021

View reviewed changes

compiler/rustc_mir/src/transform/separate_const_switch.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

cjgillot reviewed Jun 10, 2021

View reviewed changes

compiler/rustc_mir/src/transform/separate_const_switch.rs Outdated Show resolved Hide resolved

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 10, 2021

Moxinilian force-pushed the separate-const-switch branch 2 times, most recently from 78db715 to 4f965dd Compare June 20, 2021 09:43

implemented separate_const_switch MIR opt

a77e2ad

un-update itertools improve predecessor amount short-circuiting cleanup and comments somewhat improved drawing

Moxinilian force-pushed the separate-const-switch branch from c31a69f to a77e2ad Compare June 22, 2021 21:18

crlf0710 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 Jul 9, 2021

Moxinilian mentioned this pull request Jul 13, 2021

LLVM/MIR optimizations to simplify the new RFC3058 ? desugaring #85133

Closed

cjgillot reviewed Jul 18, 2021

View reviewed changes

compiler/rustc_mir/src/transform/separate_const_switch.rs Show resolved Hide resolved

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 Jul 25, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 25, 2021

bors merged commit 70f7471 into rust-lang:master Jul 25, 2021

rustbot added this to the 1.56.0 milestone Jul 25, 2021

scottmcm mentioned this pull request Jan 19, 2022

The panic branch of Some(x).unwrap() is not removed for some types #93011

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR opt: separate constant predecessors of a switch #85646

MIR opt: separate constant predecessors of a switch #85646

Moxinilian commented May 24, 2021 •

edited

Loading

rust-highfive commented May 24, 2021

cjgillot left a comment

cjgillot May 24, 2021

Moxinilian May 24, 2021

Moxinilian Jun 9, 2021

Moxinilian commented May 25, 2021 •

edited

Loading

Moxinilian commented May 25, 2021 •

edited

Loading

varkor commented May 27, 2021

nagisa commented Jun 6, 2021

Moxinilian commented Jun 9, 2021 •

edited

Loading

Moxinilian commented Jun 9, 2021

lqd commented Jun 10, 2021

rust-timer commented Jun 10, 2021

bors commented Jun 10, 2021

This comment has been minimized.

bors commented Jun 10, 2021

rust-timer commented Jun 10, 2021

rust-timer commented Jun 10, 2021

Moxinilian commented Jun 20, 2021 •

edited

Loading

nagisa commented Jun 20, 2021

Moxinilian commented Jun 20, 2021

cjgillot commented Jul 25, 2021

bors commented Jul 25, 2021

bors commented Jul 25, 2021

bors commented Jul 25, 2021

MIR opt: separate constant predecessors of a switch #85646

MIR opt: separate constant predecessors of a switch #85646

Conversation

Moxinilian commented May 24, 2021 • edited Loading

LLVM blocker

rust-highfive commented May 24, 2021

cjgillot left a comment

Choose a reason for hiding this comment

cjgillot May 24, 2021

Choose a reason for hiding this comment

Moxinilian May 24, 2021

Choose a reason for hiding this comment

Moxinilian Jun 9, 2021

Choose a reason for hiding this comment

Moxinilian commented May 25, 2021 • edited Loading

Moxinilian commented May 25, 2021 • edited Loading

varkor commented May 27, 2021

nagisa commented Jun 6, 2021

Moxinilian commented Jun 9, 2021 • edited Loading

Moxinilian commented Jun 9, 2021

lqd commented Jun 10, 2021

rust-timer commented Jun 10, 2021

bors commented Jun 10, 2021

This comment has been minimized.

bors commented Jun 10, 2021

rust-timer commented Jun 10, 2021

rust-timer commented Jun 10, 2021

Moxinilian commented Jun 20, 2021 • edited Loading

nagisa commented Jun 20, 2021

Moxinilian commented Jun 20, 2021

cjgillot commented Jul 25, 2021

bors commented Jul 25, 2021

bors commented Jul 25, 2021

bors commented Jul 25, 2021

Moxinilian commented May 24, 2021 •

edited

Loading

Moxinilian commented May 25, 2021 •

edited

Loading

Moxinilian commented May 25, 2021 •

edited

Loading

Moxinilian commented Jun 9, 2021 •

edited

Loading

Moxinilian commented Jun 20, 2021 •

edited

Loading