Extend uninhabited enum variant branch elimination to also affect fallthrough #93387

JakobDegen · 2022-01-27T18:41:36Z

The uninhabited_enum_branching mir opt eliminates branches on variants where the data is uninhabited. This change extends this pass to also ensure that the otherwise case points to a trivially unreachable bb if all inhabited variants are present in the non-otherwise branches.

I believe it was @scottmcm who said that LLVM eliminates some of this information in its SimplifyCFG pass. This is unfortunate, but this change should still be at least a small improvement in principle (I don't think it will show up on any benchmarks)

rust-highfive · 2022-01-27T18:41:39Z

r? @jackh726

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

scottmcm · 2022-01-27T19:28:24Z

Here's the comment from the person who looked into why LLVM loses this info sometimes: #85133 (comment)

That said, I do agree that making things obviously-unreachable in MIR is a good thing to do.

I'm not really qualified to review this, but I can kick off a
@bors try @rust-timer queue

rust-timer · 2022-01-27T19:28:25Z

Awaiting bors try build completion.

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

bors · 2022-01-27T19:28:32Z

⌛ Trying commit 27fa2feeb89a79f0b595199e5e5052d36c81335b with merge e8eeaa2ec2ea8a30bafceccf825f0ab85c50eaa0...

scottmcm · 2022-01-27T19:31:50Z

compiler/rustc_mir_transform/src/uninhabited_enum_branching.rs

+    let otherwise = targets.otherwise();
+    let bb = &body.basic_blocks()[otherwise];
+    if bb.statements.len() == 0
+        && bb.terminator().kind == TerminatorKind::Unreachable


I always wonder about StorageLive/Dead in these.

Would it make sense for this to be, say,

if bb.terminator().kind == TerminatorKind::Unreachable && !bb.is_cleanup && bb.statements.iter().all(|s| matches!(&s.kind, StatementKind::StorageDead(_))) {

Probably? I'll wait for Wesley to take a look

I've changed this for now since I was fixing the other thing, but can revert this if there's a reason to prefer the other way around

jackh726 · 2022-01-27T19:41:38Z

Also not qualified to review this. r? rust-lang/mir-opt

bors · 2022-01-27T21:02:11Z

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

rust-timer · 2022-01-27T21:02:13Z

Queued e8eeaa2ec2ea8a30bafceccf825f0ab85c50eaa0 with parent 21b4a9c, future comparison URL.

rust-timer · 2022-01-27T22:34:29Z

Finished benchmarking commit (e8eeaa2ec2ea8a30bafceccf825f0ab85c50eaa0): comparison url.

Summary: This benchmark run did not return any relevant results. 2 results were found to be statistically significant but too small to be relevant.

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 led to changes in compiler perf.

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

compiler/rustc_mir_transform/src/uninhabited_enum_branching.rs

...-opt/uninhabited_fallthrough_elimination.eliminate_fallthrough.UninhabitedEnumBranching.diff

…ugh. The `uninhabited_enum_branch` miropt now also checks whether the fallthrough case is inhabited, and if not will ensure that it points to an unreachable block.

…as dead

tmiasko · 2022-02-18T16:58:35Z

Thanks.

@bors r+ rollup=never

bors · 2022-02-18T16:58:36Z

📌 Commit 0783009 has been approved by tmiasko

bors · 2022-02-19T17:24:09Z

⌛ Testing commit 0783009 with merge 26381c0af57c9f901af488e59a0c2bee8de610f1...

rust-log-analyzer · 2022-02-19T19:03:30Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2022-02-19T19:03:43Z

💔 Test failed - checks-actions

tmiasko · 2022-02-19T19:25:37Z

@bors retry x86_64-apple-2 failure without logs

bors · 2022-02-20T05:24:56Z

⌛ Testing commit 0783009 with merge a6fe969...

bors · 2022-02-20T08:38:23Z

☀️ Test successful - checks-actions
Approved by: tmiasko
Pushing a6fe969 to master...

rust-timer · 2022-02-20T10:08:47Z

Finished benchmarking commit (a6fe969): comparison url.

Summary: This benchmark run did not return any relevant results. 3 results were found to be statistically significant but too small to be relevant.

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

@rustbot label: -perf-regression

rust-highfive assigned jackh726 Jan 27, 2022

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

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

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 Jan 27, 2022

scottmcm reviewed Jan 27, 2022

View reviewed changes

rust-highfive assigned wesleywiser and unassigned jackh726 Jan 27, 2022

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

tmiasko reviewed Jan 28, 2022

View reviewed changes

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

JakobDegen force-pushed the improve_partialeq branch from 27fa2fe to 90a0081 Compare January 29, 2022 01:41

This comment has been minimized.

Sign in to view

tmiasko requested changes Feb 8, 2022

View reviewed changes

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

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

JakobDegen force-pushed the improve_partialeq branch from 90a0081 to 8bc43fd Compare February 11, 2022 23:12

JakobDegen requested a review from tmiasko February 11, 2022 23:13

tmiasko reviewed Feb 12, 2022

View reviewed changes

...-opt/uninhabited_fallthrough_elimination.eliminate_fallthrough.UninhabitedEnumBranching.diff Show resolved Hide resolved

JakobDegen added 2 commits February 18, 2022 09:14

Extend uninhabited match branch optimization to also work on fallthro…

3a5c078

…ugh. The `uninhabited_enum_branch` miropt now also checks whether the fallthrough case is inhabited, and if not will ensure that it points to an unreachable block.

Add test checking that fallthrough branches are correctly identified …

0783009

…as dead

JakobDegen force-pushed the improve_partialeq branch from 8bc43fd to 0783009 Compare February 18, 2022 14:34

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 Feb 18, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2022

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 Feb 19, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 20, 2022

bors merged commit a6fe969 into rust-lang:master Feb 20, 2022

rustbot added this to the 1.61.0 milestone Feb 20, 2022

JakobDegen deleted the improve_partialeq branch March 5, 2022 02:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend uninhabited enum variant branch elimination to also affect fallthrough #93387

Extend uninhabited enum variant branch elimination to also affect fallthrough #93387

JakobDegen commented Jan 27, 2022

rust-highfive commented Jan 27, 2022

This comment has been minimized.

scottmcm commented Jan 27, 2022

rust-timer commented Jan 27, 2022

bors commented Jan 27, 2022

scottmcm Jan 27, 2022

JakobDegen Jan 27, 2022

JakobDegen Jan 29, 2022

jackh726 commented Jan 27, 2022

bors commented Jan 27, 2022

rust-timer commented Jan 27, 2022

rust-timer commented Jan 27, 2022

This comment has been minimized.

tmiasko commented Feb 18, 2022

bors commented Feb 18, 2022

bors commented Feb 19, 2022

rust-log-analyzer commented Feb 19, 2022

bors commented Feb 19, 2022

tmiasko commented Feb 19, 2022

bors commented Feb 20, 2022

bors commented Feb 20, 2022

rust-timer commented Feb 20, 2022

Extend uninhabited enum variant branch elimination to also affect fallthrough #93387

Extend uninhabited enum variant branch elimination to also affect fallthrough #93387

Conversation

JakobDegen commented Jan 27, 2022

rust-highfive commented Jan 27, 2022

This comment has been minimized.

scottmcm commented Jan 27, 2022

rust-timer commented Jan 27, 2022

bors commented Jan 27, 2022

scottmcm Jan 27, 2022

Choose a reason for hiding this comment

JakobDegen Jan 27, 2022

Choose a reason for hiding this comment

JakobDegen Jan 29, 2022

Choose a reason for hiding this comment

jackh726 commented Jan 27, 2022

bors commented Jan 27, 2022

rust-timer commented Jan 27, 2022

rust-timer commented Jan 27, 2022

This comment has been minimized.

tmiasko commented Feb 18, 2022

bors commented Feb 18, 2022

bors commented Feb 19, 2022

rust-log-analyzer commented Feb 19, 2022

bors commented Feb 19, 2022

tmiasko commented Feb 19, 2022

bors commented Feb 20, 2022

bors commented Feb 20, 2022

rust-timer commented Feb 20, 2022