mir_build: Add an extra intermediate step in MIR building for patterns by Zalathar · Pull Request #155144 · rust-lang/rust

Zalathar · 2026-04-11T08:37:00Z

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 · 2026-04-11T08:37:06Z

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

Zalathar · 2026-04-11T10:29:49Z

@bors try @rust-timer queue

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

rust-bors · 2026-04-11T12:37:16Z

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

rust-timer · 2026-04-11T14:00:18Z

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 · 2026-04-18T12:06:25Z

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 · 2026-04-18T12:06:30Z

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 · 2026-04-24T14:23:35Z

+        // Recursively squash any subpatterns into refutable `MatchPairTree` forests.
+        let mut subpairs = vec![];
+        for subpat in inter_pat.subpats {
+            MatchPairTree::squash_inter_pat(subpat, &mut subpairs, extra_data);
+        }
+
+        if let Some(testable_case) = inter_pat.testable_case {


For clarity I'd prefer the rest of the function to be in the else branch of the "or pattern" case

Nadrieril · 2026-04-24T14:30:18Z

        match_pairs: &mut Vec<Self>, // Newly-created nodes are added to this vector
        extra_data: &mut PatternExtraData<'tcx>, // Bindings/ascriptions are added here
    ) {
+        extra_data.ascriptions.extend(inter_pat.ascriptions);


Would you mind starting the function with a let InterPat { ascriptions, ....} = inter_pat with an exhaustive field list so we're sure not to forget a field?

Nadrieril · 2026-04-24T14:31:07Z

+            ascriptions: vec![],
+            is_never: inter_pat.is_never,
+        };
+        MatchPairTree::squash_inter_pat(inter_pat, &mut match_pairs, &mut extra_data);


nit: I have a preference for inlining squash_inter_pat into here, the current split took me a couple seconds to understand

squash_inter_pat is recursive (since it recursively squashes subpatterns), so I don't think it can be inlined.

oh, then I'd move the function definition inside of here too. attaching it to MatchPairTree doesn't make sense to me

In that case I would prefer to make it a top-level free function, as I think it’s too large to comfortable be an inline helper function.

But pulling it out of impl MatchPairTree is certainly fine.

Fair. Then impl InterPat maybe?

It was mainly in MatchPairTree because it replaced some code that was already in that impl block (which in turn was only there because I didn't move it out in #137875).

Now that it's a free function, I don't see any particular benefit from putting it in impl InterPat instead.

shrug, lgtm either way

Zalathar · 2026-04-25T00:41:15Z

(Rebased onto main to make things easier; no actual changes yet.)

Zalathar · 2026-04-25T01:02:33Z

+                // Or-patterns never need a place during MIR building.
+                place: None,


Note that currently we retain places for or-pattern nodes and then (I believe) never use them, so this is a deliberate change from current behaviour.

View changes since the review

Sounds good. How do you feel about making most of InterPat into an enum to distinguish the or-pattern vs non-or-pattern cases? Cause they share very little at this point

I was planning on doing something like that eventually.

I didn’t do it in my initial draft because it clashed a bit with how I had written squash_inter_pat, and I wanted to keep this initial switchover simple. But with the new structure maybe it’s worth doing now. I’ll have a poke around and try it out.

Hmm, after trying it out, I think my preference is to stick with the struct for now, with the intention to maybe make it more enum-based later on.

Zalathar · 2026-04-25T04:23:37Z

Updated some comments (diff); still considering what to do about the style feedback.

This makes it easier to perform coordinated modifications to `FlatPat::new` and `MatchPairTree::from_pattern`, because the two functions are heavily coupled.

rustbot · 2026-04-27T11:30:28Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

This splits out a separate `InterPat` data structure from `MatchPairTree`/`FlatPat`/`Candidate`, which should hopefully make it easier to modify these earlier parts of match lowering without having to simultaneously adjust many use-sites in later steps.

rustbot assigned Nadrieril Apr 11, 2026

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 11, 2026

Zalathar mentioned this pull request Apr 11, 2026

Move recursion out of MatchPairTree::for_pattern helpers #154943

Merged