Rewrite exhaustiveness in one pass #117611

Nadrieril · 2023-11-05T14:26:34Z

This is at least my 4th attempt at this in as many years x) Previous attempts were all too complicated or too slow. But we're finally here!

The previous version of the exhaustiveness algorithm computed reachability for each arm then exhaustiveness of the whole match. Since each of these steps does roughly the same things, this rewrites the algorithm to do them all in one go. I also think this makes things much simpler.

I also rewrote the documentation of the algorithm in depth. Hopefully it's up-to-date and easier to follow now. Plz comment if anything's unclear.

r? @oli-obk I think you're one of the rare other people to understand the exhaustiveness algorithm?

cc @varkor I know you're not active anymore, but if you feel like having a look you might enjoy this :D

Fixes #79307

rustbot · 2023-11-05T14:26:37Z

Could not assign reviewer from: oli-obk.
User(s) oli-obk are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

Nadrieril · 2023-11-05T14:26:41Z

@bors try @rust-timer queue

rustbot · 2023-11-05T14:26:43Z

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

bors · 2023-11-05T14:27:51Z

⌛ Trying commit 297ed32 with merge c0c929f...

Rewrite exhaustiveness in one pass This is at least my 4th attempt at this in as many years x) Previous attempts were all too complicated or too slow. But we're finally here! The previous version of the exhaustiveness algorithm computed reachability for each arm then exhaustiveness of the whole match. Since each of these steps does roughly the same things, this rewrites the algorithm to do them all in one go. I also think this makes things much simpler. I also rewrote the documentation of the algorithm in depth. Hopefully it's up-to-date and easier to follow now. Plz comment if anything's unclear. r? `@oli-obk` I think you're one of the rare other people to understand the exhaustiveness algorithm? cc `@varkor` I know you're not active anymore, but if you feel like having a look you might enjoy this :D

Nadrieril · 2023-11-05T14:45:48Z

@bors try @rust-timer queue

bors · 2023-11-05T14:46:58Z

⌛ Trying commit ab918af with merge 14c4d2f...

Rewrite exhaustiveness in one pass This is at least my 4th attempt at this in as many years x) Previous attempts were all too complicated or too slow. But we're finally here! The previous version of the exhaustiveness algorithm computed reachability for each arm then exhaustiveness of the whole match. Since each of these steps does roughly the same things, this rewrites the algorithm to do them all in one go. I also think this makes things much simpler. I also rewrote the documentation of the algorithm in depth. Hopefully it's up-to-date and easier to follow now. Plz comment if anything's unclear. r? `@oli-obk` I think you're one of the rare other people to understand the exhaustiveness algorithm? cc `@varkor` I know you're not active anymore, but if you feel like having a look you might enjoy this :D

Noratrieb · 2023-11-05T15:07:44Z

you'll have to wait until january for oli to appear again :)

Nadrieril · 2023-11-05T15:36:17Z

Ok! Thanks for the heads-up. Know anyone else familiar with exhaustiveness? @cjgillot how do you feel about it?

bors · 2023-11-05T16:09:52Z

☀️ Try build successful - checks-actions
Build commit: 14c4d2f (14c4d2ff18c46f09d9d0e40e316521523bd510b8)

cjgillot · 2023-11-05T21:33:20Z

I'll take a look during the week or next week-end.

Nadrieril · 2023-11-05T22:36:57Z

Thank you!

compiler/rustc_mir_build/src/thir/pattern/usefulness.rs

rust-timer · 2023-11-07T00:14:43Z

Finished benchmarking commit (14c4d2f): comparison URL.

Overall result: ❌✅ regressions and improvements - 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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +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)	2.3%	[2.2%, 2.4%]	6
Improvements ✅ (primary)	-1.0%	[-1.9%, -0.3%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-1.0%	[-1.9%, -0.3%]	10

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.9%	[0.6%, 1.2%]	4
Regressions ❌ (secondary)	0.6%	[0.6%, 0.6%]	1
Improvements ✅ (primary)	-1.2%	[-1.6%, -0.8%]	2
Improvements ✅ (secondary)	-5.0%	[-5.0%, -5.0%]	1
All ❌✅ (primary)	0.2%	[-1.6%, 1.2%]	6

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.5%	[-0.5%, -0.5%]	2
Improvements ✅ (secondary)	-0.6%	[-0.6%, -0.6%]	1
All ❌✅ (primary)	-0.5%	[-0.5%, -0.5%]	2

Binary size

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

Bootstrap: 638.392s -> 636.7s (-0.27%)
Artifact size: 304.54 MiB -> 304.56 MiB (0.01%)

Nadrieril · 2023-11-07T00:27:00Z

We can ignore the match-stress benchmark, it shows no change on cycles or wall-time

Nadrieril · 2023-11-07T00:31:38Z

Incidentally this fixes #79307 :D

This disentangles the row-specific tracking of `parent_row` etc from the logical operation of specialization. This means `wildcard_row` doesn't need to provide dummy values for `parent_row` etc anymore.

Nadrieril · 2023-11-22T04:00:07Z

@rustbot ready

cjgillot · 2023-11-25T18:21:50Z

@bors r+

bors · 2023-11-25T18:21:53Z

📌 Commit 273cbb7 has been approved by cjgillot

It is now in the queue for this repository.

bors · 2023-11-26T00:14:17Z

⌛ Testing commit 273cbb7 with merge ee80c8d...

bors · 2023-11-26T02:27:00Z

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

rust-timer · 2023-11-26T03:50:19Z

Finished benchmarking commit (ee80c8d): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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)	1.1%	[1.0%, 1.1%]	2
Regressions ❌ (secondary)	1.6%	[0.3%, 2.4%]	9
Improvements ✅ (primary)	-0.9%	[-2.0%, -0.2%]	11
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-0.6%	[-2.0%, 1.1%]	13

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)	2.3%	[0.8%, 4.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.1%	[-1.3%, -1.0%]	4
All ❌✅ (primary)	-	-	0

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)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 674.609s -> 672.939s (-0.25%)
Artifact size: 313.33 MiB -> 313.35 MiB (0.01%)

nnethercote · 2023-11-26T22:56:59Z

The icount regressions on secondary stress tests are balanced by improvement on primary benchmarks, which is fine.

@rustbot label: +perf-regression-triaged

…, r=<try> Exhaustiveness: Improve performance on wide matches rust-lang#118437 revealed an exponential case in exhaustiveness checking. While [exponential cases are unavoidable](https://compilercrim.es/rust-np/), this one only showed up after my rust-lang#117611 rewrite of the algorithm. I remember anticipating a case like this and dismissing it as unrealistic, but here we are :'). The tricky match is as follows: ```rust match command { BaseCommand { field01: true, .. } => {} BaseCommand { field02: true, .. } => {} BaseCommand { field03: true, .. } => {} BaseCommand { field04: true, .. } => {} BaseCommand { field05: true, .. } => {} BaseCommand { field06: true, .. } => {} BaseCommand { field07: true, .. } => {} BaseCommand { field08: true, .. } => {} BaseCommand { field09: true, .. } => {} BaseCommand { field10: true, .. } => {} // ...20 more of the same _ => {} } ``` To fix this, this PR formalizes a concept of "relevancy" (naming is hard) that was already used to decide what patterns to report. Now we track it for every row, which in wide matches like the above can drastically cut on the number of cases we explore. After this fix, the above match is checked with linear-many cases instead of exponentially-many. Fixes rust-lang#118437 r? `@cjgillot`

…, r=cjgillot Exhaustiveness: Improve complexity on some wide matches rust-lang#118437 revealed an exponential case in exhaustiveness checking. While [exponential cases are unavoidable](https://compilercrim.es/rust-np/), this one only showed up after my rust-lang#117611 rewrite of the algorithm. I remember anticipating a case like this and dismissing it as unrealistic, but here we are :'). The tricky match is as follows: ```rust match command { BaseCommand { field01: true, .. } => {} BaseCommand { field02: true, .. } => {} BaseCommand { field03: true, .. } => {} BaseCommand { field04: true, .. } => {} BaseCommand { field05: true, .. } => {} BaseCommand { field06: true, .. } => {} BaseCommand { field07: true, .. } => {} BaseCommand { field08: true, .. } => {} BaseCommand { field09: true, .. } => {} BaseCommand { field10: true, .. } => {} // ...20 more of the same _ => {} } ``` To fix this, this PR formalizes a concept of "relevancy" (naming is hard) that was already used to decide what patterns to report. Now we track it for every row, which in wide matches like the above can drastically cut on the number of cases we explore. After this fix, the above match is checked with linear-many cases instead of exponentially-many. Fixes rust-lang#118437 r? `@cjgillot`

rustbot assigned davidtwco Nov 5, 2023

This comment has been minimized.

Sign in to view

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. S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 5, 2023

This comment was marked as outdated.

Sign in to view

Nadrieril mentioned this pull request Nov 5, 2023

[Experiment] Rewrite exhaustiveness in one pass #116042

Closed

Nadrieril force-pushed the linear-pass-take-4 branch from 297ed32 to ab918af Compare November 5, 2023 14:43

This comment has been minimized.

Sign in to view

cjgillot self-assigned this Nov 5, 2023

varkor reviewed Nov 5, 2023

View reviewed changes

compiler/rustc_mir_build/src/thir/pattern/usefulness.rs Show resolved Hide resolved

davidtwco removed their assignment Nov 6, 2023

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Nov 7, 2023

Nadrieril added 5 commits November 22, 2023 03:25

Fully rework the algorithm and its explanation

cc6936d

Store wildcard row in the matrix

fd0d883

Cleanup span passing

0edb879

Clarify the Wildcard/Missing situation

41e8f58

Separate PatStack and MatrixRow

273cbb7

This disentangles the row-specific tracking of `parent_row` etc from the logical operation of specialization. This means `wildcard_row` doesn't need to provide dummy values for `parent_row` etc anymore.

Nadrieril force-pushed the linear-pass-take-4 branch from 8661717 to 273cbb7 Compare November 22, 2023 03:13

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 22, 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 Nov 25, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 26, 2023

bors merged commit ee80c8d into rust-lang:master Nov 26, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Nov 26, 2023

bors mentioned this pull request Nov 26, 2023

Exhaustiveness: reveal opaque types properly #116821

Merged

Nadrieril deleted the linear-pass-take-4 branch November 26, 2023 07:08

rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 26, 2023

This was referenced Nov 29, 2023

exhaustiveness checking hangs forever when compiling pulsar-rs #118437

Closed

Match checking has quadratic average complexity #7462

Closed

Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 10, 2023

Nadrieril mentioned this pull request Dec 10, 2023

Exhaustiveness: Improve complexity on some wide matches #118796

Merged

matthiaskrgr mentioned this pull request Jan 1, 2024

ICE: Unexpected type for constructor #119493

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite exhaustiveness in one pass #117611

Rewrite exhaustiveness in one pass #117611

Nadrieril commented Nov 5, 2023 •

edited

Loading

rustbot commented Nov 5, 2023

Nadrieril commented Nov 5, 2023

rustbot commented Nov 5, 2023

This comment has been minimized.

bors commented Nov 5, 2023

This comment was marked as outdated.

Nadrieril commented Nov 5, 2023

This comment has been minimized.

bors commented Nov 5, 2023

Noratrieb commented Nov 5, 2023

Nadrieril commented Nov 5, 2023

bors commented Nov 5, 2023

cjgillot commented Nov 5, 2023

Nadrieril commented Nov 5, 2023

rust-timer commented Nov 7, 2023

Nadrieril commented Nov 7, 2023

Nadrieril commented Nov 7, 2023

Nadrieril commented Nov 22, 2023

cjgillot commented Nov 25, 2023

bors commented Nov 25, 2023

bors commented Nov 26, 2023

bors commented Nov 26, 2023

rust-timer commented Nov 26, 2023

nnethercote commented Nov 26, 2023

Rewrite exhaustiveness in one pass #117611

Rewrite exhaustiveness in one pass #117611

Conversation

Nadrieril commented Nov 5, 2023 • edited Loading

rustbot commented Nov 5, 2023

Nadrieril commented Nov 5, 2023

rustbot commented Nov 5, 2023

This comment has been minimized.

bors commented Nov 5, 2023

This comment was marked as outdated.

Nadrieril commented Nov 5, 2023

This comment has been minimized.

bors commented Nov 5, 2023

Noratrieb commented Nov 5, 2023

Nadrieril commented Nov 5, 2023

bors commented Nov 5, 2023

cjgillot commented Nov 5, 2023

Nadrieril commented Nov 5, 2023

rust-timer commented Nov 7, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Nadrieril commented Nov 7, 2023

Nadrieril commented Nov 7, 2023

Nadrieril commented Nov 22, 2023

cjgillot commented Nov 25, 2023

bors commented Nov 25, 2023

bors commented Nov 26, 2023

bors commented Nov 26, 2023

rust-timer commented Nov 26, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

nnethercote commented Nov 26, 2023

Nadrieril commented Nov 5, 2023 •

edited

Loading