Remove SimplifyBranchSame MIR optimization

[tmiasko]

This optimization never had any positive impact on compile or runtime
performance. It is relatively complex and used to have bugs in the past.
Its implementation contains significantly more lines of code than the
number of blocks it happens to optimize out during rustc build process
(including all dependencies and the standard library).

Remove it.

[rust-highfive]

r? @lcnr

(rust_highfive has picked a reviewer for you, use r? to override)

[lcnr]

r? @oli-obk maybe, I personally don't know enough to tell if this is a good change

[wesleywiser]

I don't think we should remove this pass at this time. This pass is a pretty important part of the set of passes which try to optimize out no-op matches. Eventually, that will be used to remove the costs currently associated with the ? operator once we have inlining working and on by default. Most of the recent bugs with this pass have been due to recent work on the pass; they are not longstanding structural issues.

I'm eager to hear what @oli-obk thinks though.

[tmiasko]

I was collecting statistics about various MIR optimizations. While building
rustc at -Zmir-opt-level=1 / -Zmir-opt-level=2, this pass removes somewhere
between 60 and 210 basic blocks in total! IMHO this is not sufficient to
considier it tested, let alone useful.

[oli-obk]

This was indeed the groundwork for optimizing ? properly, which is mostly blocked on inlining now. I don't believe we should remove this pass unless we have a less complex or otherwise better replacement. I'm fine keeping it at mir-opt-level 3 until it actually helps with ? if that does not regress perf.

[tmiasko]

I find it really concerning that the criterion for introducing and enabling MIR optimizations is having no performance impact, without having to demonstrate a positive impact (performance or otherwise).

I was able to identify and minimize the rustls miscompilation introduced by SimplifyBranchSame in less than 15 minutes by examining every single mir that was changed by it (in a crate with all its dependencies). That is how rarely it applies.

[oli-obk]

It was never intended to live on its own. It was always meant to be part of helping llvm understand ?. I agree that our policy on MIR optimizations is bad, because it does not exist. We should probably set up a sync mir-opt meeting and talk about that

[wesleywiser]

I find it really concerning that the criterion for introducing and enabling MIR optimizations is having no performance impact, without having to demonstrate a positive impact (performance or otherwise).

We do have policy in this regard: rust-lang/compiler-team#319. Per that, to be enabled at mir-opt-level=1, the optimization must improve compilation time (by how much or in what situations is unspecified) and must not hurt debugability. We definitely are not enforcing this policy as strictly as we should be, I would say.

The SimplifyBranchSame pass was written specifically to work in conjunction with the SimplifyArmIdentity pass. When we disabled that pass a few months ago because of bugs, the performance regression was enough that the weekly perf triage caught it unsolicited. Whether a 3% improvement on real world crates is enough to justify being enabled by default is debatable but I would say "yes".

We should probably set up a sync mir-opt meeting and talk about that.

Yes, I agree. In particular, I would like to see us draft some policy around how optimizations are taken from experimental/unsound status to run by default.

[tmiasko]

The SimplifyBranchSame pass was written specifically to work in conjunction with the SimplifyArmIdentity pass.

Hmm, it fires roughly 5% more times with SimplifyArmIdentity enabled.

Could you run perf?

[wesleywiser]

Sure!

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 9de6df6 with merge 2eb0ebec12f360fedd04678259e7c7d4a4fc7686...

[bors]

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

[rust-timer]

Queued 2eb0ebec12f360fedd04678259e7c7d4a4fc7686 with parent 5ddef54, future comparison URL.

[rust-timer]

Finished benchmarking try commit (2eb0ebec12f360fedd04678259e7c7d4a4fc7686): 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

[tmiasko]

Looking at perf results, this pass does not improve the performance, and should be disabled by default.

Since you have some long term vision that includes this pass, I am closing this PR.