Auto merge of rust-lang#119112 - Nadrieril:remove-target_blocks-hack, r=matthewjasper

match lowering: Remove the `make_target_blocks` hack

This hack was introduced 4 years ago in [`a1d0266` (rust-lang#60730)](rust-lang@a1d0266) to improve LLVM optimization time, specifically noticed in the `encoding` benchmark. Measurements today indicate it is no longer needed.

r? `@matthewjasper`

Author: bors

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

@@ -1699,59 +1699,51 @@ 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,
-                );
-            };
+        // 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 = self.cfg.start_new_block();
+                    self.match_candidates(
+                        span,
+                        scrutinee_span,
+                        candidate_start,
+                        remainder_start,
+                        &mut *candidates,
+                        fake_borrows,
+                    );
+                    candidate_start
+                } else {
+                    *remainder_start.get_or_insert_with(|| self.cfg.start_new_block())
+                }
+            })
+            .collect();
 
-            target_blocks
-        };
+        if !candidates.is_empty() {
+            let remainder_start = remainder_start.unwrap_or_else(|| self.cfg.start_new_block());
+            self.match_candidates(
+                span,
+                scrutinee_span,
+                remainder_start,
+                otherwise_block,
+                candidates,
+                fake_borrows,
+            );
+        }
 
-        self.perform_test(span, scrutinee_span, block, &match_place, &test, make_target_blocks);
+        self.perform_test(span, scrutinee_span, block, &match_place, &test, target_blocks);
     }
 
     /// Determine the fake borrows that are needed from a set of places that

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

@@ -147,15 +147,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         }
     }
 
-    #[instrument(skip(self, make_target_blocks, place_builder), level = "debug")]
+    #[instrument(skip(self, target_blocks, place_builder), 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>,
+        target_blocks: Vec<BasicBlock>,
     ) {
         let place = place_builder.to_place(self);
         let place_ty = place.ty(&self.local_decls, self.tcx);
@@ -164,7 +164,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         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 +209,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 {
@@ -240,6 +238,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
 
             TestKind::Eq { value, ty } => {
                 let tcx = self.tcx;
+                let [success_block, fail_block] = *target_blocks else {
+                    bug!("`TestKind::Eq` should have two target blocks")
+                };
                 if let ty::Adt(def, _) = ty.kind()
                     && Some(def.did()) == tcx.lang_items().string()
                 {
@@ -280,38 +281,43 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     );
                     self.non_scalar_compare(
                         eq_block,
-                        make_target_blocks,
+                        success_block,
+                        fail_block,
                         source_info,
                         value,
                         ref_str,
                         ref_str_ty,
                     );
-                    return;
-                }
-                if !ty.is_scalar() {
+                } else 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,
+                        success_block,
+                        fail_block,
                         source_info,
                         value,
                         place,
                         ty,
                     );
-                } else if let [success, fail] = *make_target_blocks(self) {
+                } else {
                     assert_eq!(value.ty(), ty);
                     let expect = self.literal_operand(test.span, value);
                     let val = Operand::Copy(place);
-                    self.compare(block, success, fail, source_info, BinOp::Eq, expect, val);
-                } else {
-                    bug!("`TestKind::Eq` should have two target blocks");
+                    self.compare(
+                        block,
+                        success_block,
+                        fail_block,
+                        source_info,
+                        BinOp::Eq,
+                        expect,
+                        val,
+                    );
                 }
             }
 
             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 +347,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 +410,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     fn non_scalar_compare(
         &mut self,
         block: BasicBlock,
-        make_target_blocks: impl FnOnce(&mut Self) -> Vec<BasicBlock>,
+        success_block: BasicBlock,
+        fail_block: BasicBlock,
         source_info: SourceInfo,
         value: Const<'tcx>,
         mut val: Place<'tcx>,
@@ -531,9 +536,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         );
         self.diverge_from(block);
 
-        let [success_block, fail_block] = *make_target_blocks(self) else {
-            bug!("`TestKind::Eq` should have two target blocks")
-        };
         // check the result
         self.cfg.terminate(
             eq_block,

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: {

tests/mir-opt/match_test.main.SimplifyCfg-initial.after.mir

@@ -28,43 +28,43 @@ fn main() -> () {
         StorageLive(_3);
         PlaceMention(_1);
         _6 = Le(const 0_i32, _1);
-        switchInt(move _6) -> [0: bb4, otherwise: bb1];
+        switchInt(move _6) -> [0: bb3, otherwise: bb8];
     }
 
     bb1: {
-        _7 = Lt(_1, const 10_i32);
-        switchInt(move _7) -> [0: bb4, otherwise: bb2];
+        falseEdge -> [real: bb9, imaginary: bb4];
     }
 
     bb2: {
-        falseEdge -> [real: bb9, imaginary: bb6];
+        _3 = const 3_i32;
+        goto -> bb14;
     }
 
     bb3: {
-        _3 = const 3_i32;
-        goto -> bb14;
+        _4 = Le(const 10_i32, _1);
+        switchInt(move _4) -> [0: bb5, otherwise: bb7];
     }
 
     bb4: {
-        _4 = Le(const 10_i32, _1);
-        switchInt(move _4) -> [0: bb7, otherwise: bb5];
+        falseEdge -> [real: bb12, imaginary: bb6];
     }
 
     bb5: {
-        _5 = Le(_1, const 20_i32);
-        switchInt(move _5) -> [0: bb7, otherwise: bb6];
+        switchInt(_1) -> [4294967295: bb6, otherwise: bb2];
     }
 
     bb6: {
-        falseEdge -> [real: bb12, imaginary: bb8];
+        falseEdge -> [real: bb13, imaginary: bb2];
     }
 
     bb7: {
-        switchInt(_1) -> [4294967295: bb8, otherwise: bb3];
+        _5 = Le(_1, const 20_i32);
+        switchInt(move _5) -> [0: bb5, otherwise: bb4];
     }
 
     bb8: {
-        falseEdge -> [real: bb13, imaginary: bb3];
+        _7 = Lt(_1, const 10_i32);
+        switchInt(move _7) -> [0: bb3, otherwise: bb1];
     }
 
     bb9: {
@@ -83,7 +83,7 @@ fn main() -> () {
 
     bb11: {
         StorageDead(_9);
-        falseEdge -> [real: bb3, imaginary: bb6];
+        falseEdge -> [real: bb2, imaginary: bb4];
     }
 
     bb12: {