From 31bad13f82a774fa0ad3c8df350a4884cfe07e74 Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
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 | 92 +++++++++----------
.../rustc_mir_build/src/build/matches/test.rs | 42 +++++----
....match_tuple.SimplifyCfg-initial.after.mir | 14 +--
...ch_test.main.SimplifyCfg-initial.after.mir | 30 +++---
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 541b87af797..487b1f44b5e 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<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 };
+ // 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();
+ // 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();
- 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,
- );
- };
+ 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,
+ );
+ }
- target_blocks
- };
-
- 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 d1952704da3..53e5d70f946 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<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,
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 b04e09e88b8..596dcef85fd 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<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: {
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 ebb2f70a475..5bf78b6150f 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];
- }
-
- bb3: {
_3 = const 3_i32;
goto -> bb14;
}
- bb4: {
+ bb3: {
_4 = Le(const 10_i32, _1);
- switchInt(move _4) -> [0: bb7, otherwise: bb5];
+ switchInt(move _4) -> [0: bb5, otherwise: bb7];
+ }
+
+ bb4: {
+ 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: {