[MIR] Enhance the SimplifyCfg pass to merge consecutive blocks

[gereeter]

This is a rebase of @dotdash's #29838. I'm not sure that I like the last commit, as it seems a lot harder to tell that predecessors are tracked correctly when what predecessors are changing isn't written down.

[rust-highfive]

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

[dotdash]

Thanks for picking that up! I didn't have the time to take care of it yet. I'm not sure if I like the predecessor counter instead of the map though. I'm somewhat expecting people to come up with passes that will require actual predecessor lookup, and then it'd be nice if the existing code already deals with keeping the map updated.

Did you address your other concern about missed optimizations?

[gereeter]

That makes sense - that possibility was another reason that I was uncomfortable about my second commit. However, I'd be inclined to limit things to storing counts until we actually have a pass that needs the extra complexity.

My other concern about missed optimizations seemed separate from this change, so I did it in #30239 (all it took was changing simplify_branches to use the DFS mechanism).

[nrc]

r? @dotdash

[dotdash]

As the original author, I don't think I should do the review for this.

r? @nikomatsakis -- you expressed some interest in reviewing my original PR.

FWIW, my only concern here really is that re-introducing the actual predecessor map later might be harder.

[bluss]

Are the graphs large enough that a BitVector would be better for the seen map?

[gereeter]

I don't think they are large enough for it to make a big difference. However, they might be small enough that using a u64 or even a u32 with no allocation might work (with a fallback to Vec or BitVec).

Regardless, I think such a change should be in a separate PR.

[nikomatsakis]

It'd be interesting to instrument the compiler and find out what's the largest number of blocks we see in building rustc at least.

[nikomatsakis]

I'll take a look. For various reasons I've gotten pretty behind on reviews/e-mail/everything, but I'm working backwards from the most recent stuff. I remember that for the previous PR I was a bit unsure if this change was strictly better --- I thought there might be some cases that were optimized by the older approach that are not by the newer approach? Or do you think otherwise.

[nikomatsakis]

Nit: I think this would be prettier like this

let other_data = mem::replace(mir.basic_block_data_mut(target), BasicBlockData { ... });

[nikomatsakis]

Not sure what I was worried about. This seems like a simple extension. I'm inclined to r+ modulo the nits above.

[nikomatsakis]

@gereeter do you think you'll be able to address the nits I raised above, or are you too busy at the moment?

[gereeter]

@nikomatsakis Sorry for the delay - school just finished up for the semester, so I should have more time.

It just occurred to me that this, as currently implemented, can devolve into O(n log n) behavior. For example, if we have

A -> B -> C -> D

then a single pass will merge B onto the end of A and D onto the end of C, but will not actually merge AB and CD. A second pass will catch this, but this takes more work than just directly merging everything into A. I'll post a fix shortly.

[gereeter]

Okay, nits and performance issue fixed.

[nikomatsakis]

So I think it's clear we need to devise some unit tests for these optimizations! I imagine a macro that lets you relatively easily write MIR code by hand and then observe the result.

[Aatch]

Related to the comments on #30239, it seems to me that you could just make merge_consecutive_blocks self-contained, working until a fixpoint. It already keeps the predecessor map up-to-date, so just make sure the unreachable blocks have already been removed before running it, then remove any newly-unreachable blocks afterwards.

It's also pointless to run simplify_branches after this, as it won't introduce any new branches so simplify_branches could probably be made self-contained too. This would make SimplifyCfg's run_on_mir look something like this:

simplify_branches();
remove_dead_blocks();
merge_consecutive_blocks();
remove_dead_blocks();

[nikomatsakis]

Hmm, at first read, what @Aatch said made sense to me.

[gereeter]

@Aatch That makes lots of sense. Moreover,

• After merge_consecutive_blocks runs, there will be no live blocks with exactly 1 predecessor and for which that predecessor has a Goto terminator. Therefore, we don't even need to iterate merge_consecutive_blocks.
• We don't actually need to update the predecessor map in merge_consecutive_blocks. The only place that we do this is when shortcutting empty blocks, but merge_consecutive_blocks makes empty blocks dead (so they won't be merged into), and the final target at the end of the chain never could be merged into anything anyway.

[dylanmckay]

Related to #26496.

[nikomatsakis]

@gereeter so, given @Aatch's comments etc, do you plan to adjust this PR? Just trying to figure out the current status.

[bors]

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

[alexcrichton]

Closing due to inactivity, but feel free to resubmit with comments addressed!