mir_build: Add an extra intermediate step in MIR building for patterns

[Zalathar]

View all comments

This is an attempt to partly decouple the data structures created by match_pair.rs from the data structures that are ultimately consumed by the main part of match lowering.

In some ways this is a reversal from #137875. That PR succeeded in removing the TestCase::Irrefutable variant, by taking a pre-existing “simplification” step and fusing it directly into MatchPairTree::for_pattern. Unfortunately, in doing so it also reinforced a very high degree of coupling between the transformations performed in match_pair, and the data structures used by later steps.

My hope is that these changes will make it easier for follow-up work to further separate decision-making from MIR building when lowering patterns.

r? Nadrieril

[rustbot]

Nadrieril is not on the review rotation at the moment.
They may take a while to respond.

[Zalathar]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 76367e4 (76367e4db9c5ebe2cd39a24f067d772b200f4158, parent: c29effdf79bdea928ea366a2a820ed15c1de5f43)

[rust-timer]

Finished benchmarking commit (76367e4): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.1%, 0.1%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.2%	[-0.2%, -0.2%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 2.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.8%	[0.9%, 6.8%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.9%	[-3.9%, -3.9%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -9.8%, secondary -4.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-9.8%	[-20.6%, -3.1%]	13
Improvements ✅ (secondary)	-4.3%	[-8.0%, -2.5%]	4
All ❌✅ (primary)	-9.8%	[-20.6%, -3.1%]	13

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 491.148s -> 509.241s (3.68%)
Artifact size: 394.19 MiB -> 394.20 MiB (0.00%)

[Zalathar]

Marking this as ready for consideration.

I'm not entirely confident about the name “InterPat”, and I'm aware that this change doesn't yet demonstrate any clear benefit on its own, but I think it's a worthwhile step towards hopefully being able to disentangle match-lowering a bit more.

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[Nadrieril]

I like this, the separation of concerns is very clean. I'm on board, I only have a couple nits. Agreed that InterPat isn't an amazing name but it's used in a single file so who cares really.

View changes since this review

[Nadrieril]

LGTM!

@bors r+

[rust-bors]

📌 Commit b4f8806 has been approved by Nadrieril

It is now in the queue for this repository.

[JonathanBrouwer]

@bors rollup=maybe
Perf is noise

[panstromek]

for #157558

@rust-timer build e6eba30

[rust-timer]

Finished benchmarking commit (e6eba30): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.1%, 0.1%]	1
Regressions ❌ (secondary)	0.1%	[0.1%, 0.2%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.1%, 0.1%]	1

Max RSS (memory usage)

Results (primary 2.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

Results (secondary -3.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.0%	[-3.9%, -2.4%]	3
All ❌✅ (primary)	-	-	0

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 514.705s -> 517.685s (0.58%)
Artifact size: 401.25 MiB -> 400.79 MiB (-0.12%)

[panstromek]

This caused the regression in #157558. Looks like the pre-merge run didn't mark a similarly sized regression in html5ever as significant but something of that magnitude was already there.

This doesn't look like that big of a deal to me, but it might be worth a second look just in case this is unexpected.

[Nadrieril]

Indeed yeah, that type of delta is well within the "can completely ignore" range, as far as I'm used to do things.