From 31bad13f82a774fa0ad3c8df350a4884cfe07e74 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Tue, 19 Dec 2023 11:32:29 +0100 Subject: [PATCH] Remove the `make_target_blocks` hack It was introduced 4 years ago in a1d0266878793bc8 to improve LLVM optimization time. Measurements today indicate it is no longer needed. --- .../rustc_mir_build/src/build/matches/mod.rs | 94 +++++++++---------- .../rustc_mir_build/src/build/matches/test.rs | 42 +++++---- ....match_tuple.SimplifyCfg-initial.after.mir | 14 +-- ...ch_test.main.SimplifyCfg-initial.after.mir | 28 +++--- 4 files changed, 86 insertions(+), 92 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index 541b87af7977b..487b1f44b5e27 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/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 { - // 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 diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index d1952704da3ce..53e5d70f946eb 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -147,7 +147,7 @@ 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, @@ -155,7 +155,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block: BasicBlock, place_builder: &PlaceBuilder<'tcx>, test: &Test<'tcx>, - make_target_blocks: impl FnOnce(&mut Self) -> Vec, + target_blocks: Vec, ) { 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, + 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, diff --git a/tests/mir-opt/exponential_or.match_tuple.SimplifyCfg-initial.after.mir b/tests/mir-opt/exponential_or.match_tuple.SimplifyCfg-initial.after.mir index b04e09e88b8dd..596dcef85fd21 100644 --- a/tests/mir-opt/exponential_or.match_tuple.SimplifyCfg-initial.after.mir +++ b/tests/mir-opt/exponential_or.match_tuple.SimplifyCfg-initial.after.mir @@ -38,22 +38,22 @@ fn match_tuple(_1: (u32, bool, Option, 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: { diff --git a/tests/mir-opt/match_test.main.SimplifyCfg-initial.after.mir b/tests/mir-opt/match_test.main.SimplifyCfg-initial.after.mir index ebb2f70a47587..5bf78b6150f1a 100644 --- a/tests/mir-opt/match_test.main.SimplifyCfg-initial.after.mir +++ b/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: {