Auto merge of #119031 - Nadrieril:two-phase-match-lowering, r=<try>

[Experiment] Play with match lowering

Match lowering to MIR has the reputation of being the most intricate piece of the compiler, and after banging my head on it for a bit I sure hope there isn't anything more intricate somewhere else. It's good quality code but I hope to unentangle it. This PR is me wrestling with it and asking `rustc-timer` for its opinion.

r? `@ghost`

Author: bors

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -1063,6 +1063,27 @@ pub(crate) struct Test<'tcx> {
 #[derive(Copy, Clone, Debug)]
 pub(crate) struct ArmHasGuard(pub(crate) bool);
 
+/// A single step in the match algorithm.
+pub(crate) struct MatchAutomatonStep<'a, 'c, 'pat, 'tcx> {
+    /// Perform this test...
+    test: Test<'tcx>,
+    /// ... on this place...
+    place: PlaceBuilder<'tcx>,
+    /// ... depending on the result, branch to one of these candidate lists...
+    target_candidates: Vec<Vec<&'a mut Candidate<'pat, 'tcx>>>,
+    /// ... if it doesn't match, continue with these.
+    remaining_candidates: &'a mut [&'c mut Candidate<'pat, 'tcx>],
+}
+
+impl<'b, 'c, 'pat, 'tcx> std::fmt::Debug for MatchAutomatonStep<'b, 'c, 'pat, 'tcx> {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("MatchAutomatonStep")
+            .field("test", &self.test)
+            .field("place", &self.place)
+            .finish()
+    }
+}
+
 ///////////////////////////////////////////////////////////////////////////
 // Main matching algorithm
 
@@ -1085,7 +1106,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// exhaustive in Rust. But during processing we sometimes divide
     /// up the list of candidates and recurse with a non-exhaustive
     /// list. This is important to keep the size of the generated code
-    /// under control. See [`Builder::test_candidates`] for more details.
+    /// under control. See [`Builder::build_test_step`] for more details.
     ///
     /// If `fake_borrows` is `Some`, then places which need fake borrows
     /// will be added to it.
@@ -1321,7 +1342,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     }
 
     /// Tests a candidate where there are only or-patterns left to test, or
-    /// forwards to [Builder::test_candidates].
+    /// forwards to [Builder::build_test_step].
     ///
     /// Given a pattern `(P | Q, R | S)` we (in principle) generate a CFG like
     /// so:
@@ -1389,14 +1410,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         match first_candidate.match_pairs[0].pattern.kind {
             PatKind::Or { .. } => (),
             _ => {
-                self.test_candidates(
-                    span,
-                    scrutinee_span,
-                    candidates,
-                    block,
-                    otherwise_block,
-                    fake_borrows,
-                );
+                let step = self.build_test_step(candidates, fake_borrows);
+                self.perform_test(span, scrutinee_span, block, otherwise_block, fake_borrows, step);
                 return;
             }
         }
@@ -1449,11 +1464,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
     ) {
         debug!("candidate={:#?}\npats={:#?}", candidate, pats);
-        let mut or_candidates: Vec<_> = pats
+        candidate.subcandidates = pats
             .iter()
             .map(|pat| Candidate::new(place.clone(), pat, candidate.has_guard, self))
             .collect();
-        let mut or_candidate_refs: Vec<_> = or_candidates.iter_mut().collect();
+        let mut or_candidate_refs: Vec<_> = candidate.subcandidates.iter_mut().collect();
         let otherwise = if candidate.otherwise_block.is_some() {
             &mut candidate.otherwise_block
         } else {
@@ -1467,7 +1482,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             &mut or_candidate_refs,
             fake_borrows,
         );
-        candidate.subcandidates = or_candidates;
         self.merge_trivial_subcandidates(candidate, self.source_info(or_span));
     }
 
@@ -1626,15 +1640,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// In addition to avoiding exponential-time blowups, this algorithm
     /// also has the nice property that each guard and arm is only generated
     /// once.
-    fn test_candidates<'pat, 'b, 'c>(
+    fn build_test_step<'pat, 'b, 'c>(
         &mut self,
-        span: Span,
-        scrutinee_span: Span,
         mut candidates: &'b mut [&'c mut Candidate<'pat, 'tcx>],
-        block: BasicBlock,
-        otherwise_block: &mut Option<BasicBlock>,
         fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
-    ) {
+    ) -> MatchAutomatonStep<'b, 'c, 'pat, 'tcx> {
         // extract the match-pair from the highest priority candidate
         let match_pair = &candidates.first().unwrap().match_pairs[0];
         let mut test = self.test(match_pair);
@@ -1673,7 +1683,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // those N possible outcomes, create a (initially empty)
         // vector of candidates. Those are the candidates that still
         // apply if the test has that particular outcome.
-        debug!("test_candidates: test={:?} match_pair={:?}", test, match_pair);
+        debug!("build_test_step: test={:?} match_pair={:?}", test, match_pair);
         let mut target_candidates: Vec<Vec<&mut Candidate<'pat, 'tcx>>> = vec![];
         target_candidates.resize_with(test.targets(), Default::default);
 
@@ -1699,59 +1709,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         debug!("tested_candidates: {}", total_candidate_count - candidates.len());
         debug!("untested_candidates: {}", candidates.len());
 
-        // HACK(matthewjasper) This is a closure so that we can let the test
-        // create its blocks before the rest of the match. This currently
-        // improves the speed of llvm when optimizing long string literal
-        // matches
-        let make_target_blocks = move |this: &mut Self| -> Vec<BasicBlock> {
-            // The block that we should branch to if none of the
-            // `target_candidates` match. This is either the block where we
-            // start matching the untested candidates if there are any,
-            // otherwise it's the `otherwise_block`.
-            let remainder_start = &mut None;
-            let remainder_start =
-                if candidates.is_empty() { &mut *otherwise_block } else { remainder_start };
-
-            // For each outcome of test, process the candidates that still
-            // apply. Collect a list of blocks where control flow will
-            // branch if one of the `target_candidate` sets is not
-            // exhaustive.
-            let target_blocks: Vec<_> = target_candidates
-                .into_iter()
-                .map(|mut candidates| {
-                    if !candidates.is_empty() {
-                        let candidate_start = this.cfg.start_new_block();
-                        this.match_candidates(
-                            span,
-                            scrutinee_span,
-                            candidate_start,
-                            remainder_start,
-                            &mut *candidates,
-                            fake_borrows,
-                        );
-                        candidate_start
-                    } else {
-                        *remainder_start.get_or_insert_with(|| this.cfg.start_new_block())
-                    }
-                })
-                .collect();
-
-            if !candidates.is_empty() {
-                let remainder_start = remainder_start.unwrap_or_else(|| this.cfg.start_new_block());
-                this.match_candidates(
-                    span,
-                    scrutinee_span,
-                    remainder_start,
-                    otherwise_block,
-                    candidates,
-                    fake_borrows,
-                );
-            };
-
-            target_blocks
-        };
-
-        self.perform_test(span, scrutinee_span, block, &match_place, &test, make_target_blocks);
+        MatchAutomatonStep {
+            test,
+            place: match_place,
+            remaining_candidates: candidates,
+            target_candidates,
+        }
     }
 
     /// Determine the fake borrows that are needed from a set of places that

compiler/rustc_mir_build/src/build/matches/test.rs

@@ -8,7 +8,7 @@
 use crate::build::expr::as_place::PlaceBuilder;
 use crate::build::matches::{Candidate, MatchPair, Test, TestKind};
 use crate::build::Builder;
-use rustc_data_structures::fx::FxIndexMap;
+use rustc_data_structures::fx::{FxIndexMap, FxIndexSet};
 use rustc_hir::{LangItem, RangeEnd};
 use rustc_index::bit_set::BitSet;
 use rustc_middle::mir::*;
@@ -23,6 +23,8 @@ use rustc_target::abi::VariantIdx;
 
 use std::cmp::Ordering;
 
+use super::MatchAutomatonStep;
+
 impl<'a, 'tcx> Builder<'a, 'tcx> {
     /// Identifies what test is needed to decide if `match_pair` is applicable.
     ///
@@ -147,24 +149,82 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         }
     }
 
-    #[instrument(skip(self, make_target_blocks, place_builder), level = "debug")]
+    #[instrument(skip(self), level = "debug")]
     pub(super) fn perform_test(
         &mut self,
         match_start_span: Span,
         scrutinee_span: Span,
         block: BasicBlock,
-        place_builder: &PlaceBuilder<'tcx>,
-        test: &Test<'tcx>,
-        make_target_blocks: impl FnOnce(&mut Self) -> Vec<BasicBlock>,
+        otherwise_block: &mut Option<BasicBlock>,
+        fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
+        step: MatchAutomatonStep<'_, '_, '_, 'tcx>,
     ) {
+        let test = &step.test;
+        let place_builder = &step.place;
+        // HACK(matthewjasper) This is a closure so that we can let the test
+        // create its blocks before the rest of the match. This currently
+        // improves the speed of llvm when optimizing long string literal
+        // matches
+        let make_target_blocks = move |this: &mut Self| -> Vec<BasicBlock> {
+            // The block that we should branch to if none of the
+            // `target_candidates` match. This is either the block where we
+            // start matching the untested candidates if there are any,
+            // otherwise it's the `otherwise_block`.
+            let remainder_start = &mut None;
+            let remainder_start = if step.remaining_candidates.is_empty() {
+                &mut *otherwise_block
+            } else {
+                remainder_start
+            };
+
+            // For each outcome of test, process the candidates that still
+            // apply. Collect a list of blocks where control flow will
+            // branch if one of the `target_candidate` sets is not
+            // exhaustive.
+            let target_blocks: Vec<_> = step
+                .target_candidates
+                .into_iter()
+                .map(|mut candidates| {
+                    if !candidates.is_empty() {
+                        let candidate_start = this.cfg.start_new_block();
+                        this.match_candidates(
+                            match_start_span,
+                            scrutinee_span,
+                            candidate_start,
+                            remainder_start,
+                            &mut *candidates,
+                            fake_borrows,
+                        );
+                        candidate_start
+                    } else {
+                        *remainder_start.get_or_insert_with(|| this.cfg.start_new_block())
+                    }
+                })
+                .collect();
+
+            if !step.remaining_candidates.is_empty() {
+                let remainder_start = remainder_start.unwrap_or_else(|| this.cfg.start_new_block());
+                this.match_candidates(
+                    match_start_span,
+                    scrutinee_span,
+                    remainder_start,
+                    otherwise_block,
+                    step.remaining_candidates,
+                    fake_borrows,
+                );
+            };
+
+            target_blocks
+        };
+        let target_blocks = make_target_blocks(self);
+
         let place = place_builder.to_place(self);
         let place_ty = place.ty(&self.local_decls, self.tcx);
         debug!(?place, ?place_ty,);
 
         let source_info = self.source_info(test.span);
         match test.kind {
             TestKind::Switch { adt_def, ref variants } => {
-                let target_blocks = make_target_blocks(self);
                 // Variants is a BitVec of indexes into adt_def.variants.
                 let num_enum_variants = adt_def.variants().len();
                 debug_assert_eq!(target_blocks.len(), num_enum_variants + 1);
@@ -210,7 +270,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             }
 
             TestKind::SwitchInt { switch_ty, ref options } => {
-                let target_blocks = make_target_blocks(self);
                 let terminator = if *switch_ty.kind() == ty::Bool {
                     assert!(!options.is_empty() && options.len() <= 2);
                     let [first_bb, second_bb] = *target_blocks else {
@@ -280,7 +339,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     );
                     self.non_scalar_compare(
                         eq_block,
-                        make_target_blocks,
+                        target_blocks,
                         source_info,
                         value,
                         ref_str,
@@ -291,15 +350,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 if !ty.is_scalar() {
                     // Use `PartialEq::eq` instead of `BinOp::Eq`
                     // (the binop can only handle primitives)
-                    self.non_scalar_compare(
-                        block,
-                        make_target_blocks,
-                        source_info,
-                        value,
-                        place,
-                        ty,
-                    );
-                } else if let [success, fail] = *make_target_blocks(self) {
+                    self.non_scalar_compare(block, target_blocks, source_info, value, place, ty);
+                } else if let [success, fail] = *target_blocks {
                     assert_eq!(value.ty(), ty);
                     let expect = self.literal_operand(test.span, value);
                     let val = Operand::Copy(place);
@@ -311,7 +363,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
             TestKind::Range(ref range) => {
                 let lower_bound_success = self.cfg.start_new_block();
-                let target_blocks = make_target_blocks(self);
 
                 // Test `val` by computing `lo <= val && val <= hi`, using primitive comparisons.
                 // FIXME: skip useless comparison when the range is half-open.
@@ -341,8 +392,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             }
 
             TestKind::Len { len, op } => {
-                let target_blocks = make_target_blocks(self);
-
                 let usize_ty = self.tcx.types.usize;
                 let actual = self.temp(usize_ty, test.span);
 
@@ -406,7 +455,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     fn non_scalar_compare(
         &mut self,
         block: BasicBlock,
-        make_target_blocks: impl FnOnce(&mut Self) -> Vec<BasicBlock>,
+        target_blocks: Vec<BasicBlock>,
         source_info: SourceInfo,
         value: Const<'tcx>,
         mut val: Place<'tcx>,
@@ -531,7 +580,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         );
         self.diverge_from(block);
 
-        let [success_block, fail_block] = *make_target_blocks(self) else {
+        let [success_block, fail_block] = *target_blocks else {
             bug!("`TestKind::Eq` should have two target blocks")
         };
         // check the result

tests/mir-opt/exponential_or.match_tuple.SimplifyCfg-initial.after.mir

@@ -38,22 +38,22 @@ fn match_tuple(_1: (u32, bool, Option<i32>, u32)) -> u32 {
 
     bb4: {
         _5 = Le(const 6_u32, (_1.3: u32));
-        switchInt(move _5) -> [0: bb6, otherwise: bb5];
+        switchInt(move _5) -> [0: bb5, otherwise: bb7];
     }
 
     bb5: {
-        _6 = Le((_1.3: u32), const 9_u32);
-        switchInt(move _6) -> [0: bb6, otherwise: bb8];
+        _3 = Le(const 13_u32, (_1.3: u32));
+        switchInt(move _3) -> [0: bb1, otherwise: bb6];
     }
 
     bb6: {
-        _3 = Le(const 13_u32, (_1.3: u32));
-        switchInt(move _3) -> [0: bb1, otherwise: bb7];
+        _4 = Le((_1.3: u32), const 16_u32);
+        switchInt(move _4) -> [0: bb1, otherwise: bb8];
     }
 
     bb7: {
-        _4 = Le((_1.3: u32), const 16_u32);
-        switchInt(move _4) -> [0: bb1, otherwise: bb8];
+        _6 = Le((_1.3: u32), const 9_u32);
+        switchInt(move _6) -> [0: bb5, otherwise: bb8];
     }
 
     bb8: {