Deduplicate unreachable blocks, for real this time #110569

saethlin · 2023-04-20T01:06:42Z

In #106428 (in particular 41eda69) we noticed that inlining unreachable_unchecked can produce duplicate unreachable blocks. So we improved two MIR optimizations: SimplifyCfg was given a simplify to deduplicate unreachable blocks, then InstCombine was given a combiner to deduplicate switch targets that point at the same block. The problem is that change doesn't actually work.

Our current pass order is

SimplifyCfg (does nothing relevant to this situation)
Inline (produces multiple unreachable blocks)
InstCombine (doesn't do anything here, oops)
SimplifyCfg (produces the duplicate SwitchTargets that InstCombine is looking for)

So in here, I have factored out the specific function from InstCombine and placed it inside the simplify that produces the case it is looking for. This should ensure that it runs in the scenario it was designed for.

Fixes #110551
r? @cjgillot

rustbot · 2023-04-20T01:06:50Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

cjgillot · 2023-04-20T09:37:54Z

tests/mir-opt/instcombine_duplicate_switch_targets_e2e.rs

@@ -0,0 +1,16 @@
+// compile-flags: -Zmir-opt-level=2 -Zinline-mir
+// ignore-debug: standard library debug assertions add a panic that breaks this optimization


Blessing the ignore-debug tests requires to recompile everything, which is tedious.
Can you use the intrinsic directly and cook your own unreachable_unchecked?

I'm really wary of doing things like that. Modifying the mir-opt test for the convenience of testing is what caused this regression in the first place, and this test is already somewhat removed from the code that was reported to not optimize.

cjgillot · 2023-04-20T09:40:28Z

compiler/rustc_mir_transform/src/simplify.rs

                    *target = self.duplicates[0];
                }
            }

+            if optimization_was_applied {


Is this heavy enough to warrant the short circuit?
Could this cause us to miss cases where combining would be useful, but the branches are already deduplicated?
match { A => {}, B | C => unreachable }

Or do you rely on the call in instcombine?

When I wrote this I think I didn't realize how rare this code should be, since we already only get here this if there are multiple unreachable blocks in the Body. And I'm quite sure I do not want to be relying on InstCombine.

compiler/rustc_mir_transform/src/instcombine.rs

cjgillot · 2023-04-20T09:44:01Z

tests/mir-opt/instcombine_duplicate_switch_targets_e2e.rs

@@ -0,0 +1,16 @@
+// compile-flags: -Zmir-opt-level=2 -Zinline-mir
+// ignore-debug: standard library debug assertions add a panic that breaks this optimization
+#![crate_type = "lib"]


Unrelated: should we make mir-opt tests "lib" by default, to avoid to list all the cases in a fn main everywhere?

If we can. The codegen tests could do with the same treatment; I pulled this style of testing from them. There's a #![crate_type = "lib"] in nearly every single codegen test file.

…oduces them

saethlin · 2023-04-21T01:16:58Z

It seems prudent to
@bors try @rust-timer queue

bors · 2023-04-21T01:17:08Z

⌛ Trying commit 8ec49ad with merge 31ffe61ee8b251efc5bd2f3561d22d594d5a0cab...

bors · 2023-04-21T02:56:59Z

☀️ Try build successful - checks-actions
Build commit: 31ffe61ee8b251efc5bd2f3561d22d594d5a0cab (31ffe61ee8b251efc5bd2f3561d22d594d5a0cab)

rust-timer · 2023-04-21T04:12:30Z

Finished benchmarking commit (31ffe61ee8b251efc5bd2f3561d22d594d5a0cab): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

cjgillot · 2023-04-21T11:09:26Z

@bors r+

bors · 2023-04-21T11:09:28Z

📌 Commit 8ec49ad has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-04-21T15:08:05Z

⌛ Testing commit 8ec49ad with merge 4a03f14...

bors · 2023-04-21T17:28:30Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 4a03f14 to master...

rust-timer · 2023-04-21T18:58:04Z

Finished benchmarking commit (4a03f14): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

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)	-	-	0
Regressions ❌ (secondary)	3.5%	[3.5%, 3.5%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.1%, -1.1%]	1
All ❌✅ (primary)	-	-	0

Cycles

This benchmark run did not return any relevant results for this metric.

rustbot assigned cjgillot Apr 20, 2023

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 20, 2023

This comment has been minimized.

Sign in to view

Demonstrate the bug

b658050

saethlin force-pushed the mir-pass-cooperation branch from 4867f69 to eead45d Compare April 20, 2023 03:01

cjgillot reviewed Apr 20, 2023

View reviewed changes

Run combine_duplicate_switch_targets after the simplification that pr…

8ec49ad

…oduces them

saethlin force-pushed the mir-pass-cooperation branch from eead45d to 8ec49ad Compare April 21, 2023 00:40

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 21, 2023

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 21, 2023

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 21, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 21, 2023

bors merged commit 4a03f14 into rust-lang:master Apr 21, 2023

rustbot added this to the 1.71.0 milestone Apr 21, 2023

jyn514 mentioned this pull request Apr 26, 2023

Use matches by None in is_none method on Option #110456

Closed

saethlin deleted the mir-pass-cooperation branch April 28, 2023 16:43

apiraino mentioned this pull request Oct 11, 2023

format_args! does not respect integer literals' type #115423

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate unreachable blocks, for real this time #110569

Deduplicate unreachable blocks, for real this time #110569

saethlin commented Apr 20, 2023 •

edited

Loading

rustbot commented Apr 20, 2023

This comment has been minimized.

cjgillot Apr 20, 2023

saethlin Apr 21, 2023

cjgillot Apr 20, 2023

cjgillot Apr 20, 2023

saethlin Apr 21, 2023 •

edited

Loading

cjgillot Apr 20, 2023

saethlin Apr 21, 2023

saethlin commented Apr 21, 2023

This comment has been minimized.

bors commented Apr 21, 2023

bors commented Apr 21, 2023

This comment has been minimized.

rust-timer commented Apr 21, 2023

cjgillot commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

rust-timer commented Apr 21, 2023

		@@ -0,0 +1,16 @@
		// compile-flags: -Zmir-opt-level=2 -Zinline-mir
		// ignore-debug: standard library debug assertions add a panic that breaks this optimization

Deduplicate unreachable blocks, for real this time #110569

Deduplicate unreachable blocks, for real this time #110569

Conversation

saethlin commented Apr 20, 2023 • edited Loading

rustbot commented Apr 20, 2023

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin commented Apr 21, 2023

This comment has been minimized.

bors commented Apr 21, 2023

bors commented Apr 21, 2023

This comment has been minimized.

rust-timer commented Apr 21, 2023

Overall result: no relevant changes - no action needed

cjgillot commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

bors commented Apr 21, 2023

rust-timer commented Apr 21, 2023

Overall result: no relevant changes - no action needed

saethlin commented Apr 20, 2023 •

edited

Loading

saethlin Apr 21, 2023 •

edited

Loading