Disable SimplifyToExp in MatchBranchSimplification by dianqk · Pull Request #124156 · rust-lang/rust

dianqk · 2024-04-19T10:28:02Z

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

To fully resolve this issue, my plan is:

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

r? @Nilstrieb or @oli-obk

rustbot · 2024-04-19T10:28:11Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

RalfJung · 2024-04-19T10:29:10Z

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

dianqk · 2024-04-19T10:31:08Z

tests/mir-opt/switch_to_self.rs

@@ -1,3 +1,4 @@
+//@ unit-test: MatchBranchSimplification


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

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".

Because the default compiler flag is -O.

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?

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

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

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

dianqk · 2024-04-19T10:31:52Z

tests/mir-opt/issues/issue_75439.rs

@@ -1,9 +1,10 @@
+//@ unit-test: MatchBranchSimplification


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

RalfJung · 2024-04-19T10:36:58Z

tests/mir-opt/instsimplify/ub_check.rs

@@ -1,4 +1,5 @@
 //@ unit-test: InstSimplify
+//@ should-fail Broken due to https://github.com/rust-lang/rust/issues/124150.


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

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

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 · 2024-04-19T10:44:30Z

@bors try @rust-timer queue

bors · 2024-04-19T10:45:40Z

⌛ Trying commit 2d3178b with merge e7a5d6f...

Disable MatchBranchSimplification Due to the miscompilation mentioned in rust-lang#124150, We need to disable MatchBranchSimplification temporarily. To fully resolve this issue, my plan is: 1. Disable MatchBranchSimplification (this PR). 2. Remove all potentially unclear transforms in rust-lang#124122. 3. Gradually add back the removed transforms (possibly multiple PRs). r? `@Nilstrieb` or `@oli-obk`

rustbot · 2024-04-19T10:59:19Z

Some changes occurred in coverage tests.

cc @Zalathar

dianqk · 2024-04-19T11:29:13Z

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

RalfJung · 2024-04-19T11:37:57Z

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 · 2024-04-19T12:43:37Z

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 · 2024-04-19T20:19:59Z

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

dianqk · 2024-04-19T21:44:44Z

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 · 2024-04-20T06:21:53Z

@bors r+

bors · 2024-04-20T06:21:56Z

📌 Commit b52be28 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2024-04-20T08:47:10Z

⌛ Testing commit b52be28 with merge a61b14d...

bors · 2024-04-20T10:49:33Z

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

rust-timer · 2024-04-20T13:17:56Z

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%)

rustbot assigned Noratrieb Apr 19, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 19, 2024

dianqk commented Apr 19, 2024

View reviewed changes

RalfJung reviewed Apr 19, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 19, 2024

This comment has been minimized.

Sign in to view

dianqk force-pushed the disable-match_branches branch from 2d3178b to ca30ce6 Compare April 19, 2024 10:59

This comment has been minimized.

Sign in to view

dianqk force-pushed the disable-match_branches branch from ca30ce6 to 86a51cd Compare April 19, 2024 12:55

dianqk changed the title ~~Disable MatchBranchSimplification~~ Disable SimplifyToExp in MatchBranchSimplification Apr 19, 2024

This comment has been minimized.

Sign in to view

dianqk force-pushed the disable-match_branches branch from 86a51cd to e2f8ec6 Compare April 19, 2024 13:14

Disable SimplifyToExp in MatchBranchSimplification

b52be28

dianqk force-pushed the disable-match_branches branch from e2f8ec6 to b52be28 Compare April 19, 2024 21:38

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 Apr 20, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 20, 2024

bors merged commit a61b14d into rust-lang:master Apr 20, 2024

rustbot added this to the 1.79.0 milestone Apr 20, 2024

dianqk deleted the disable-match_branches branch April 20, 2024 11:01

RalfJung mentioned this pull request Apr 20, 2024

Miscompilation due to MatchBranchSimplification MIR pass mixing up discriminants #124150

Closed

dianqk mentioned this pull request Oct 30, 2024

Panic in nightly 1.83.0 and 1.84.0 with opt-level >= 1 when unwrapping Some variant #132353

Closed

		@@ -1,4 +1,5 @@
		//@ unit-test: InstSimplify
		//@ should-fail Broken due to https://github.com/rust-lang/rust/issues/124150.

Uh oh!

Conversation

dianqk commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Apr 19, 2024

Uh oh!

RalfJung commented Apr 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Apr 19, 2024

Uh oh!

This comment has been minimized.

bors commented Apr 19, 2024

Uh oh!

This comment has been minimized.

rustbot commented Apr 19, 2024

Uh oh!

dianqk commented Apr 19, 2024

Uh oh!

RalfJung commented Apr 19, 2024

Uh oh!

This comment has been minimized.

dianqk commented Apr 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

RalfJung commented Apr 19, 2024

Uh oh!

dianqk commented Apr 19, 2024

Uh oh!

RalfJung commented Apr 20, 2024

Uh oh!

bors commented Apr 20, 2024

Uh oh!

bors commented Apr 20, 2024

Uh oh!

bors commented Apr 20, 2024

Uh oh!

rust-timer commented Apr 20, 2024

Overall result: ❌ regressions - no action needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dianqk commented Apr 19, 2024 •

edited

Loading

RalfJung Apr 19, 2024 •

edited

Loading

dianqk commented Apr 19, 2024 •

edited

Loading