UnreachableProp: Preserve unreachable branches for multiple targets #99762

Nilstrieb · 2022-07-26T13:25:56Z

Before, UnreachablePropagation removed all unreachable branches. This was a pessimization, as it removed information about reachability that was used later in the optimization pipeline.

For example, this code

pub enum Two { A, B }
pub fn identity(x: Two) -> Two {
    match x {
        Two::A => Two::A,
        Two::B => Two::B,
    }
}

basically has switchInt() -> [0: 0, 1: 1, otherwise: unreachable] for the match. This allows it to be transformed into a simple x. If we remove the unreachable branch, the transformation becomes illegal.

This was the problem keeping UnreachablePropagation from being enabled, so we can enable it now.

Something similar already happened in #77800, but it did not show a perf improvement there. Let's try it again anyways!

Fixes #68105, although that issue has been fixed for a long time (see #77680).

rust-highfive · 2022-07-26T13:26:00Z

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

rustbot · 2022-07-26T13:26:01Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

eddyb · 2022-07-26T16:31:04Z

basically has switchInt() -> [0: 0, 1: 1, otherwise: unreachable] for the match. This allows it to be transformed into a simple x. If we remove the unreachable branch, the transformation becomes illegal.

Isn't the issue that it becomes -> [0: 0, otherwise: 1]? But -> [0: 0, 1: 1] would work, if it was equivalent in making the "otherwise" case UB.

And if that's indeed the problem, we could theoretically encode otherwise: unreachable as "missing otherwise", in a similar fashion to how never-returning calls have no destination Place for their returned value.

JakobDegen · 2022-07-26T16:48:26Z

@eddyb agreed, especially because otherwise edges are Option<BasicBlock> in most other places too. I think we should change that separately of this PR though

JakobDegen · 2022-07-26T18:08:58Z

@bors try @rust-timer queue

rust-timer · 2022-07-26T18:09:00Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-07-26T18:09:07Z

⌛ Trying commit b100b4de35c0f64b160da79c678e3719281ca810 with merge a7fcf124a326d3a6b66d44389da7b1b49d5493ac...

bors · 2022-07-26T20:34:02Z

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

rust-timer · 2022-07-26T20:34:04Z

Queued a7fcf124a326d3a6b66d44389da7b1b49d5493ac with parent c9b3183, future comparison URL.

rust-timer · 2022-07-26T22:08:44Z

Finished benchmarking commit (a7fcf124a326d3a6b66d44389da7b1b49d5493ac): comparison url.

Instruction count

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

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.2%	2.2%	2
Improvements 🎉 (primary)	-1.9%	-2.3%	6
Improvements 🎉 (secondary)	-2.5%	-2.8%	2
All 😿🎉 (primary)	-1.9%	-2.3%	6

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.7%	-3.7%	1
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

Nilstrieb · 2022-07-26T22:14:21Z

That's not bad, but also not great. I could try to also look at unreachable_unchecked in the pass as was suggested by people somewhere already (#77800 (comment)).

JakobDegen · 2022-07-26T22:17:29Z

We shouldn't special case the intrinsic in this pass - we should instead add it to the lower intrinsics pass we have already (if it's not in there)

Nilstrieb · 2022-07-27T08:13:50Z

The lower intrinsics pass does indeed lower unreachable already, so no improvements can be made there (assuming it runs before this pass, which it probably does).

oli-obk · 2022-07-27T11:26:52Z

compiler/rustc_mir_transform/src/unreachable_prop.rs

+                // }
+                //
+                // This generates a `switchInt() -> [0: 0, 1: 1, otherwise: unreachable]`, which allows us or LLVM to
+                // turn it into just `x` later. Without the unreachable, such a transformation would be illegal.


This statement is only true because we don't have enough range information available, right?

(Also: please add a codegen test showing this opt)

src/test/codegen/match-optimizes-away.rs is the test for this, and found this issue in the opt originally. And this doesn't necessarily have to do with range information only, it could also be an unreachable_unchecked the user wrote themselves to enable another optimization. Deleting unreachable branches like this just isn't a good idea because it loses information.

This statement is only true because we don't have enough range information available, right?

Potentially, but IMO it's a bad idea to turn this:

match x { Enum::A => 1, Enum::B => 2, Enum::C => 3, // Not in the user's code, but added for switchInt's "otherwise" target: // _ => unreachable_unchecked(), }

into:

match x { Enum::A => 1, Enum::B => 2, _ => 3, }

It's erasing the refinement that the third case is only reachable when x is Enum::C.
You can recompute this refinement with an analysis, but why erase it in the first place?

See also #99762 (comment) where I suggest a solution (make otherwise optional).

Nilstrieb · 2022-07-29T19:43:16Z

And the merge conflicts are fixed

compiler/rustc_mir_transform/src/unreachable_prop.rs

bors · 2022-08-04T17:20:35Z

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

JakobDegen · 2022-08-08T21:32:03Z

@bors try @rust-timer queue

rust-timer · 2022-08-08T21:32:05Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-08-08T21:32:14Z

⌛ Trying commit ace7e18a3e510b6e9851f073a64701ae63cc568c with merge 8a6fa0c7e08655ea76a5f9071ddeccf6ebe68ec2...

bors · 2022-08-08T23:04:34Z

☀️ Try build successful - checks-actions
Build commit: 8a6fa0c7e08655ea76a5f9071ddeccf6ebe68ec2 (8a6fa0c7e08655ea76a5f9071ddeccf6ebe68ec2)

rust-timer · 2022-08-08T23:04:35Z

Queued 8a6fa0c7e08655ea76a5f9071ddeccf6ebe68ec2 with parent f03ce30, future comparison URL.

rust-timer · 2022-08-09T00:20:27Z

Finished benchmarking commit (8a6fa0c7e08655ea76a5f9071ddeccf6ebe68ec2): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	1.4%	1.5%	6
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-1.9%	-1.9%	1
All 😿🎉 (primary)	N/A	N/A	0

Cycles

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

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

Before, UnreachablePropagation removed all unreachable branches. This was a pessimization, as it removed information about reachability that was used later in the optimization pipeline. For example, this code ```rust pub enum Two { A, B } pub fn identity(x: Two) -> Two { match x { Two::A => Two::A, Two::B => Two::B, } } ``` basically has `switchInt() -> [0: 0, 1: 1, otherwise: unreachable]` for the match. This allows it to be transformed into a simple `x`. If we remove the unreachable branch, the transformation becomes illegal.

It was disabled because of pathological behaviour of LLVM in some benchmarks. As of rust-lang#77680, this has been fixed. The problem there was that it caused pessimizations in some cases. These have now been fixed as well.

oli-obk · 2022-08-22T09:33:07Z

curious ctfe stress test regression.

238,739,914  ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::TypeFolder>::fold_ty
 44,827,316  ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::FallibleTypeFolder>::try_fold_ty
 31,450,109  ???:<rustc_middle::ty::instance::Instance>::resolve_opt_const_arg
-16,515,772  ???:<rustc_middle::ty::subst::SubstFolder as rustc_middle::ty::fold::TypeFolder>::fold_const
  2,324,998  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::run
  2,098,038  ???:<rustc_middle::ty::context::TyCtxt>::intern_type_list
 -1,848,888  ???:<rustc_middle::ty::Ty>::fn_sig
 -1,406,500  /build/glibc-sMfBJT/glibc-2.31/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:__memcpy_avx_unaligned_erms
   -748,524  ???:<rustc_middle::ty::Ty as rustc_middle::ty::fold::TypeSuperFoldable>::super_fold_with::<rustc_middle::ty::subst::SubstFolder>
    393,216  ???:<rustc_const_eval::interpret::eval_context::InterpCx<rustc_const_eval::const_eval::machine::CompileTimeInterpreter>>::eval_fn_call

seems ok to me, but weird that it doesn't show up in the details of perfbots webpage.

oli-obk · 2022-08-22T09:47:28Z

@bors r+

bors · 2022-08-22T09:47:30Z

📌 Commit 18bfcd3 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2022-08-22T20:28:19Z

⌛ Testing commit 18bfcd3 with merge 015a824...

bors · 2022-08-22T23:09:28Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 015a824 to master...

rust-timer · 2022-08-23T00:25:31Z

Finished benchmarking commit (015a824): 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)	3.0%	[3.0%, 3.0%]	1
Regressions ❌ (secondary)	2.7%	[2.2%, 3.4%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	3.0%	[3.0%, 3.0%]	1

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)	2.4%	[2.1%, 2.7%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

the arithmetic mean of the percent change ↩ ↩²
number of relevant changes ↩ ↩²

bjorn3 · 2022-08-23T06:09:32Z

compiler/rustc_mir_transform/src/unreachable_prop.rs

+                TerminatorKind::Unreachable
+            } else if is_unreachable(otherwise) {
+                // If there are multiple targets, don't delete unreachable branches (like an unreachable otherwise)
+                // unless otherwise is unrachable, in which case deleting a normal branch causes it to be merged with


*unreachable

rust-highfive assigned oli-obk Jul 26, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 26, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2022

Nilstrieb force-pushed the unreachable-prop branch from abc735d to 4e6a828 Compare July 26, 2022 13:33

This comment has been minimized.

Sign in to view

Nilstrieb force-pushed the unreachable-prop branch from 4e6a828 to 7eaab22 Compare July 26, 2022 15:36

Nilstrieb force-pushed the unreachable-prop branch from 7eaab22 to b100b4d Compare July 26, 2022 17:13

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 26, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 26, 2022

oli-obk reviewed Jul 27, 2022

View reviewed changes

Nilstrieb force-pushed the unreachable-prop branch from b100b4d to 6ec07a8 Compare July 29, 2022 18:42

JakobDegen requested changes Jul 30, 2022

View reviewed changes

compiler/rustc_mir_transform/src/unreachable_prop.rs Outdated Show resolved Hide resolved

compiler/rustc_mir_transform/src/unreachable_prop.rs Show resolved Hide resolved

Nilstrieb force-pushed the unreachable-prop branch from 6ec07a8 to ace7e18 Compare August 7, 2022 13:41

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 8, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 9, 2022

Nilstrieb added 3 commits August 21, 2022 21:15

Enable UnreachablePropagation under mir-opt-level >= 2

bc79557

It was disabled because of pathological behaviour of LLVM in some benchmarks. As of rust-lang#77680, this has been fixed. The problem there was that it caused pessimizations in some cases. These have now been fixed as well.

bless mir-opt tests

18bfcd3

Nilstrieb force-pushed the unreachable-prop branch from ace7e18 to 18bfcd3 Compare August 21, 2022 19:23

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 Aug 22, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2022

bors merged commit 015a824 into rust-lang:master Aug 22, 2022

rustbot added this to the 1.65.0 milestone Aug 22, 2022

Nilstrieb deleted the unreachable-prop branch August 23, 2022 04:18

bjorn3 reviewed Aug 23, 2022

View reviewed changes

matthiaskrgr mentioned this pull request Jan 7, 2024

ICE: effects, gce: difference in caller_arg caller_abi layout #119701

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UnreachableProp: Preserve unreachable branches for multiple targets #99762

UnreachableProp: Preserve unreachable branches for multiple targets #99762

Nilstrieb commented Jul 26, 2022 •

edited

rust-highfive commented Jul 26, 2022

rustbot commented Jul 26, 2022

This comment has been minimized.

eddyb commented Jul 26, 2022

JakobDegen commented Jul 26, 2022

JakobDegen commented Jul 26, 2022

rust-timer commented Jul 26, 2022

bors commented Jul 26, 2022

bors commented Jul 26, 2022

rust-timer commented Jul 26, 2022

rust-timer commented Jul 26, 2022

Nilstrieb commented Jul 26, 2022

JakobDegen commented Jul 26, 2022

Nilstrieb commented Jul 27, 2022

oli-obk Jul 27, 2022 •

edited

oli-obk Jul 27, 2022

Nilstrieb Jul 27, 2022

eddyb Jul 27, 2022

Nilstrieb commented Jul 29, 2022

bors commented Aug 4, 2022

JakobDegen commented Aug 8, 2022

rust-timer commented Aug 8, 2022

bors commented Aug 8, 2022

bors commented Aug 8, 2022

rust-timer commented Aug 8, 2022

rust-timer commented Aug 9, 2022

oli-obk commented Aug 22, 2022

oli-obk commented Aug 22, 2022

bors commented Aug 22, 2022

bors commented Aug 22, 2022

bors commented Aug 22, 2022

rust-timer commented Aug 23, 2022

bjorn3 Aug 23, 2022

Nilstrieb Aug 23, 2022

UnreachableProp: Preserve unreachable branches for multiple targets #99762

UnreachableProp: Preserve unreachable branches for multiple targets #99762

Conversation

Nilstrieb commented Jul 26, 2022 • edited

rust-highfive commented Jul 26, 2022

rustbot commented Jul 26, 2022

This comment has been minimized.

eddyb commented Jul 26, 2022

JakobDegen commented Jul 26, 2022

JakobDegen commented Jul 26, 2022

rust-timer commented Jul 26, 2022

bors commented Jul 26, 2022

bors commented Jul 26, 2022

rust-timer commented Jul 26, 2022

rust-timer commented Jul 26, 2022

Footnotes

Nilstrieb commented Jul 26, 2022

JakobDegen commented Jul 26, 2022

Nilstrieb commented Jul 27, 2022

oli-obk Jul 27, 2022 • edited

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

Nilstrieb commented Jul 29, 2022

bors commented Aug 4, 2022

JakobDegen commented Aug 8, 2022

rust-timer commented Aug 8, 2022

bors commented Aug 8, 2022

bors commented Aug 8, 2022

rust-timer commented Aug 8, 2022

rust-timer commented Aug 9, 2022

Footnotes

oli-obk commented Aug 22, 2022

oli-obk commented Aug 22, 2022

bors commented Aug 22, 2022

bors commented Aug 22, 2022

bors commented Aug 22, 2022

rust-timer commented Aug 23, 2022

Overall result: no relevant changes - no action needed

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nilstrieb commented Jul 26, 2022 •

edited

oli-obk Jul 27, 2022 •

edited