Disable SimplifyToExp in MatchBranchSimplification

[DianQK]

Due to the miscompilation mentioned in #124150, We need to disable MatchBranchSimplification temporarily.

To fully resolve this issue, my plan is:

1. Disable SimplifyToExp in MatchBranchSimplification (this PR).
2. Remove all potentially unclear transforms in Don't perform unsigned comparisons for signed integers  #124122.
3. Gradually add back the removed transforms (possibly multiple PRs).

r? @Nilstrieb or @oli-obk

[rustbot]

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[RalfJung]

This one is easy to review. :)
r=me when CI is green.

[DianQK]

I think it's okay to change it for unit testing since the MIR output is the same.

[RalfJung]

I'm surprised the test didn't already have this declaration, if it was meant to test a particular pass.

But also, unit-test is a very non-self-explaining name for "please run only and specifically this pass".

[DianQK]

Because the default compiler flag is -O.

[RalfJung]

Yeah I know it doesn't need the declaration to test default-on opts. But when a test tests a specific opt, wouldn't it be good to add that declaration even if it is not needed?

[DianQK]

There are probably similar test cases in the repository. ;) Perhaps there was no support for unit testing in the past?

[DianQK]

For fixing the miscompilation, I hope to make as few changes as possible. This can go into a separate PR now.

[RalfJung]

Oh yeah sure this was more of a general comment. Sorry for being unclear.

[DianQK]

The same as tests/mir-opt/switch_to_self.rs.

[RalfJung]

Why does this affect this test, when #120614 did not change this test?

[DianQK]

This was already accomplished by the existing SimplifyToIf. I just did a refactor in #120614.

[RalfJung]

Oh, so this is not equivalent to reverting #120614, it disables more than that.

In that case, hard for me to say whether it's appropriate to disable the entire pass.

[RalfJung]

@bors try @rust-timer queue

[bors]

⌛ Trying commit 2d3178b with merge e7a5d6f...

[rustbot]

Some changes occurred in coverage tests.

cc @Zalathar

[DianQK]

If the perf result is unacceptable, I would consider revert the PR or just disable SimplifyToExp.

[RalfJung]

or just disable SimplifyToExp.

If there's an easy way to disable just the affected part of this pass, then let's just do that?

[DianQK]

or just disable SimplifyToExp.

If there's an easy way to disable just the affected part of this pass, then let's just do that?

I can write the following code:

if tcx.sess.opts.unstable_opts.unsound_mir_opts
    && SimplifyToExp::default().simplify(tcx, body, bb_idx, param_env).is_some()
{
    should_cleanup = true;
    continue;
}

But I've never seen code like this before.

Edit: The code below works fine for me.

if false && SimplifyToExp::default().simplify(tcx, body, bb_idx, param_env).is_some()
{
    should_cleanup = true;
    continue;
}

[RalfJung]

Why not use tcx.sess.opts.unstable_opts.unsound_mir_opts? Seems reasonable to me.

[DianQK]

Why not use tcx.sess.opts.unstable_opts.unsound_mir_opts? Seems reasonable to me.

I'm just worried it might be a bit weird because I've never seen this kind of code before.
Done. But I didn't add -Zunsound-mir-opts to the test case because then we could see that the modified code was working.

[RalfJung]

@bors r+

[bors]

📌 Commit b52be28 has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit b52be28 with merge a61b14d...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing a61b14d to master...

[rust-timer]

Finished benchmarking commit (a61b14d): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.7%	[3.2%, 13.5%]	3
Regressions ❌ (secondary)	2.9%	[2.9%, 2.9%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	6.7%	[3.2%, 13.5%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.0%	[-1.0%, -1.0%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.1%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.0%	[-0.0%, -0.0%]	4
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-0.0%, 0.1%]	6

Bootstrap: 670.395s -> 670.71s (0.05%)
Artifact size: 315.30 MiB -> 315.28 MiB (-0.01%)