From 832b23ffcfae702c333915bf2c25493f9f62ebb5 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 1 Mar 2024 19:07:08 +0100 Subject: [PATCH 1/5] Tiny missed simplification --- compiler/rustc_mir_build/src/build/matches/test.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index 8ce7461747b40..d811141f50ff7 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -603,17 +603,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { }) } - (&TestKind::If, TestCase::Constant { value }) => { + (TestKind::If, TestCase::Constant { value }) => { fully_matched = true; let value = value.try_eval_bool(self.tcx, self.param_env).unwrap_or_else(|| { span_bug!(test.span, "expected boolean value but got {value:?}") }); Some(value as usize) } - (&TestKind::If, _) => { - fully_matched = false; - None - } ( &TestKind::Len { len: test_len, op: BinOp::Eq }, @@ -714,6 +710,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ( TestKind::Switch { .. } | TestKind::SwitchInt { .. } + | TestKind::If | TestKind::Len { .. } | TestKind::Range { .. } | TestKind::Eq { .. }, From 3d3b321c60f6ce1ac59edf0706c083aa7fbd1e83 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Thu, 29 Feb 2024 00:52:03 +0100 Subject: [PATCH 2/5] Use an enum instead of manually tracking indices for `target_blocks` --- .../rustc_mir_build/src/build/matches/mod.rs | 34 +++-- .../rustc_mir_build/src/build/matches/test.rs | 117 ++++++++++-------- .../building/issue_49232.main.built.after.mir | 8 +- ...fg-initial.after-ElaborateDrops.after.diff | 15 +-- ...fg-initial.after-ElaborateDrops.after.diff | 15 +-- 5 files changed, 112 insertions(+), 77 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index daa0349789ed6..aea52fc497f0d 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -1160,6 +1160,19 @@ pub(crate) struct Test<'tcx> { kind: TestKind<'tcx>, } +/// The branch to be taken after a test. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] +enum TestBranch<'tcx> { + /// Success branch, used for tests with two possible outcomes. + Success, + /// Branch corresponding to this constant. + Constant(Const<'tcx>, u128), + /// Branch corresponding to this variant. + Variant(VariantIdx), + /// Failure branch for tests with two possible outcomes, and "otherwise" branch for other tests. + Failure, +} + /// `ArmHasGuard` is a wrapper around a boolean flag. It indicates whether /// a match arm has a guard expression attached to it. #[derive(Copy, Clone, Debug)] @@ -1636,11 +1649,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { match_place: &PlaceBuilder<'tcx>, test: &Test<'tcx>, mut candidates: &'b mut [&'c mut Candidate<'pat, 'tcx>], - ) -> (&'b mut [&'c mut Candidate<'pat, 'tcx>], Vec>>) { + ) -> ( + &'b mut [&'c mut Candidate<'pat, 'tcx>], + FxIndexMap, Vec<&'b mut Candidate<'pat, 'tcx>>>, + ) { // For each of the N possible outcomes, create a (initially empty) vector of candidates. // Those are the candidates that apply if the test has that particular outcome. - let mut target_candidates: Vec>> = vec![]; - target_candidates.resize_with(test.targets(), Default::default); + let mut target_candidates: FxIndexMap<_, Vec<&mut Candidate<'pat, 'tcx>>> = + test.targets().into_iter().map(|branch| (branch, Vec::new())).collect(); let total_candidate_count = candidates.len(); @@ -1648,11 +1664,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // point we may encounter a candidate where the test is not relevant; at that point, we stop // sorting. while let Some(candidate) = candidates.first_mut() { - let Some(idx) = self.sort_candidate(&match_place, &test, candidate) else { + let Some(branch) = self.sort_candidate(&match_place, &test, candidate) else { break; }; let (candidate, rest) = candidates.split_first_mut().unwrap(); - target_candidates[idx].push(candidate); + target_candidates[&branch].push(candidate); candidates = rest; } @@ -1797,9 +1813,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // 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 + let target_blocks: FxIndexMap<_, _> = target_candidates .into_iter() - .map(|mut candidates| { + .map(|(branch, mut candidates)| { if !candidates.is_empty() { let candidate_start = self.cfg.start_new_block(); self.match_candidates( @@ -1809,9 +1825,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { remainder_start, &mut *candidates, ); - candidate_start + (branch, candidate_start) } else { - remainder_start + (branch, remainder_start) } }) .collect(); diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index d811141f50ff7..d003ae8d803ce 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -6,7 +6,7 @@ // the candidates based on the result. use crate::build::expr::as_place::PlaceBuilder; -use crate::build::matches::{Candidate, MatchPair, Test, TestCase, TestKind}; +use crate::build::matches::{Candidate, MatchPair, Test, TestBranch, TestCase, TestKind}; use crate::build::Builder; use rustc_data_structures::fx::FxIndexMap; use rustc_hir::{LangItem, RangeEnd}; @@ -129,11 +129,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { block: BasicBlock, place_builder: &PlaceBuilder<'tcx>, test: &Test<'tcx>, - target_blocks: Vec, + target_blocks: FxIndexMap, BasicBlock>, ) { let place = place_builder.to_place(self); let place_ty = place.ty(&self.local_decls, self.tcx); - debug!(?place, ?place_ty,); + debug!(?place, ?place_ty); + let target_block = |branch| target_blocks[&branch]; let source_info = self.source_info(test.span); match test.kind { @@ -141,20 +142,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // 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); - let otherwise_block = *target_blocks.last().unwrap(); + let otherwise_block = target_block(TestBranch::Failure); let tcx = self.tcx; let switch_targets = SwitchTargets::new( adt_def.discriminants(tcx).filter_map(|(idx, discr)| { if variants.contains(idx) { debug_assert_ne!( - target_blocks[idx.index()], + target_block(TestBranch::Variant(idx)), otherwise_block, "no candidates for tested discriminant: {discr:?}", ); - Some((discr.val, target_blocks[idx.index()])) + Some((discr.val, target_block(TestBranch::Variant(idx)))) } else { debug_assert_eq!( - target_blocks[idx.index()], + target_block(TestBranch::Variant(idx)), otherwise_block, "found candidates for untested discriminant: {discr:?}", ); @@ -185,9 +186,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { TestKind::SwitchInt { ref options } => { // The switch may be inexhaustive so we have a catch-all block debug_assert_eq!(options.len() + 1, target_blocks.len()); - let otherwise_block = *target_blocks.last().unwrap(); + let otherwise_block = target_block(TestBranch::Failure); let switch_targets = SwitchTargets::new( - options.values().copied().zip(target_blocks), + options + .iter() + .map(|(&val, &bits)| (bits, target_block(TestBranch::Constant(val, bits)))), otherwise_block, ); let terminator = TerminatorKind::SwitchInt { @@ -198,18 +201,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } TestKind::If => { - let [false_bb, true_bb] = *target_blocks else { - bug!("`TestKind::If` should have two targets") - }; - let terminator = TerminatorKind::if_(Operand::Copy(place), true_bb, false_bb); + debug_assert_eq!(target_blocks.len(), 2); + let success_block = target_block(TestBranch::Success); + let fail_block = target_block(TestBranch::Failure); + let terminator = + TerminatorKind::if_(Operand::Copy(place), success_block, fail_block); self.cfg.terminate(block, self.source_info(match_start_span), terminator); } TestKind::Eq { value, ty } => { let tcx = self.tcx; - let [success_block, fail_block] = *target_blocks else { - bug!("`TestKind::Eq` should have two target blocks") - }; + debug_assert_eq!(target_blocks.len(), 2); + let success_block = target_block(TestBranch::Success); + let fail_block = target_block(TestBranch::Failure); if let ty::Adt(def, _) = ty.kind() && Some(def.did()) == tcx.lang_items().string() { @@ -286,9 +290,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } TestKind::Range(ref range) => { - let [success, fail] = *target_blocks else { - bug!("`TestKind::Range` should have two target blocks"); - }; + debug_assert_eq!(target_blocks.len(), 2); + let success = target_block(TestBranch::Success); + let fail = target_block(TestBranch::Failure); // Test `val` by computing `lo <= val && val <= hi`, using primitive comparisons. let val = Operand::Copy(place); @@ -333,15 +337,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // expected = let expected = self.push_usize(block, source_info, len); - let [true_bb, false_bb] = *target_blocks else { - bug!("`TestKind::Len` should have two target blocks"); - }; + debug_assert_eq!(target_blocks.len(), 2); + let success_block = target_block(TestBranch::Success); + let fail_block = target_block(TestBranch::Failure); // result = actual == expected OR result = actual < expected // branch based on result self.compare( block, - true_bb, - false_bb, + success_block, + fail_block, source_info, op, Operand::Move(actual), @@ -526,10 +530,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// Given that we are performing `test` against `test_place`, this job /// sorts out what the status of `candidate` will be after the test. See - /// `test_candidates` for the usage of this function. The returned index is - /// the index that this candidate should be placed in the - /// `target_candidates` vec. The candidate may be modified to update its - /// `match_pairs`. + /// `test_candidates` for the usage of this function. The candidate may + /// be modified to update its `match_pairs`. /// /// So, for example, if this candidate is `x @ Some(P0)` and the `Test` is /// a variant test, then we would modify the candidate to be `(x as @@ -556,7 +558,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { test_place: &PlaceBuilder<'tcx>, test: &Test<'tcx>, candidate: &mut Candidate<'pat, 'tcx>, - ) -> Option { + ) -> Option> { // Find the match_pair for this place (if any). At present, // afaik, there can be at most one. (In the future, if we // adopted a more general `@` operator, there might be more @@ -576,7 +578,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ) => { assert_eq!(adt_def, tested_adt_def); fully_matched = true; - Some(variant_index.as_usize()) + Some(TestBranch::Variant(variant_index)) } // If we are performing a switch over integers, then this informs integer @@ -584,12 +586,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // // FIXME(#29623) we could use PatKind::Range to rule // things out here, in some cases. - (TestKind::SwitchInt { options }, TestCase::Constant { value }) + (TestKind::SwitchInt { options }, &TestCase::Constant { value }) if is_switch_ty(match_pair.pattern.ty) => { fully_matched = true; - let index = options.get_index_of(value).unwrap(); - Some(index) + let bits = options.get(&value).unwrap(); + Some(TestBranch::Constant(value, *bits)) } (TestKind::SwitchInt { options }, TestCase::Range(range)) => { fully_matched = false; @@ -599,7 +601,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { not_contained.then(|| { // No switch values are contained in the pattern range, // so the pattern can be matched only if this test fails. - options.len() + TestBranch::Failure }) } @@ -608,7 +610,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let value = value.try_eval_bool(self.tcx, self.param_env).unwrap_or_else(|| { span_bug!(test.span, "expected boolean value but got {value:?}") }); - Some(value as usize) + Some(if value { TestBranch::Success } else { TestBranch::Failure }) } ( @@ -620,14 +622,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // on true, min_len = len = $actual_length, // on false, len != $actual_length fully_matched = true; - Some(0) + Some(TestBranch::Success) } (Ordering::Less, _) => { // test_len < pat_len. If $actual_len = test_len, // then $actual_len < pat_len and we don't have // enough elements. fully_matched = false; - Some(1) + Some(TestBranch::Failure) } (Ordering::Equal | Ordering::Greater, true) => { // This can match both if $actual_len = test_len >= pat_len, @@ -639,7 +641,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // test_len != pat_len, so if $actual_len = test_len, then // $actual_len != pat_len. fully_matched = false; - Some(1) + Some(TestBranch::Failure) } } } @@ -653,20 +655,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // $actual_len >= test_len = pat_len, // so we can match. fully_matched = true; - Some(0) + Some(TestBranch::Success) } (Ordering::Less, _) | (Ordering::Equal, false) => { // test_len <= pat_len. If $actual_len < test_len, // then it is also < pat_len, so the test passing is // necessary (but insufficient). fully_matched = false; - Some(0) + Some(TestBranch::Success) } (Ordering::Greater, false) => { // test_len > pat_len. If $actual_len >= test_len > pat_len, // then we know we won't have a match. fully_matched = false; - Some(1) + Some(TestBranch::Failure) } (Ordering::Greater, true) => { // test_len < pat_len, and is therefore less @@ -680,12 +682,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { (TestKind::Range(test), &TestCase::Range(pat)) => { if test.as_ref() == pat { fully_matched = true; - Some(0) + Some(TestBranch::Success) } else { fully_matched = false; // If the testing range does not overlap with pattern range, // the pattern can be matched only if this test fails. - if !test.overlaps(pat, self.tcx, self.param_env)? { Some(1) } else { None } + if !test.overlaps(pat, self.tcx, self.param_env)? { + Some(TestBranch::Failure) + } else { + None + } } } (TestKind::Range(range), &TestCase::Constant { value }) => { @@ -693,7 +699,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { if !range.contains(value, self.tcx, self.param_env)? { // `value` is not contained in the testing range, // so `value` can be matched only if this test fails. - Some(1) + Some(TestBranch::Failure) } else { None } @@ -704,7 +710,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { if test_val == case_val => { fully_matched = true; - Some(0) + Some(TestBranch::Success) } ( @@ -747,18 +753,29 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } -impl Test<'_> { - pub(super) fn targets(&self) -> usize { +impl<'tcx> Test<'tcx> { + pub(super) fn targets(&self) -> Vec> { match self.kind { - TestKind::Eq { .. } | TestKind::Range(_) | TestKind::Len { .. } | TestKind::If => 2, + TestKind::Eq { .. } | TestKind::Range(_) | TestKind::Len { .. } | TestKind::If => { + vec![TestBranch::Success, TestBranch::Failure] + } TestKind::Switch { adt_def, .. } => { // While the switch that we generate doesn't test for all // variants, we have a target for each variant and the // otherwise case, and we make sure that all of the cases not // specified have the same block. - adt_def.variants().len() + 1 + adt_def + .variants() + .indices() + .map(|idx| TestBranch::Variant(idx)) + .chain([TestBranch::Failure]) + .collect() } - TestKind::SwitchInt { ref options } => options.len() + 1, + TestKind::SwitchInt { ref options } => options + .iter() + .map(|(val, bits)| TestBranch::Constant(*val, *bits)) + .chain([TestBranch::Failure]) + .collect(), } } } diff --git a/tests/mir-opt/building/issue_49232.main.built.after.mir b/tests/mir-opt/building/issue_49232.main.built.after.mir index d09a1748a8b36..166e28ce51d42 100644 --- a/tests/mir-opt/building/issue_49232.main.built.after.mir +++ b/tests/mir-opt/building/issue_49232.main.built.after.mir @@ -25,7 +25,7 @@ fn main() -> () { StorageLive(_3); _3 = const true; PlaceMention(_3); - switchInt(_3) -> [0: bb4, otherwise: bb6]; + switchInt(_3) -> [0: bb6, otherwise: bb4]; } bb3: { @@ -34,7 +34,8 @@ fn main() -> () { } bb4: { - falseEdge -> [real: bb8, imaginary: bb6]; + _0 = const (); + goto -> bb13; } bb5: { @@ -42,8 +43,7 @@ fn main() -> () { } bb6: { - _0 = const (); - goto -> bb13; + falseEdge -> [real: bb8, imaginary: bb4]; } bb7: { diff --git a/tests/mir-opt/match_arm_scopes.complicated_match.panic-abort.SimplifyCfg-initial.after-ElaborateDrops.after.diff b/tests/mir-opt/match_arm_scopes.complicated_match.panic-abort.SimplifyCfg-initial.after-ElaborateDrops.after.diff index 619fda339a6a9..307f7105dd2f1 100644 --- a/tests/mir-opt/match_arm_scopes.complicated_match.panic-abort.SimplifyCfg-initial.after-ElaborateDrops.after.diff +++ b/tests/mir-opt/match_arm_scopes.complicated_match.panic-abort.SimplifyCfg-initial.after-ElaborateDrops.after.diff @@ -42,11 +42,15 @@ } bb2: { -- switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb4]; +- switchInt((_2.0: bool)) -> [0: bb4, otherwise: bb3]; + switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb17]; } bb3: { +- falseEdge -> [real: bb20, imaginary: bb4]; +- } +- +- bb4: { StorageLive(_15); _15 = (_2.1: bool); StorageLive(_16); @@ -55,12 +59,8 @@ + goto -> bb16; } - bb4: { -- falseEdge -> [real: bb20, imaginary: bb3]; -- } -- - bb5: { -- falseEdge -> [real: bb13, imaginary: bb4]; +- falseEdge -> [real: bb13, imaginary: bb3]; - } - - bb6: { @@ -68,6 +68,7 @@ - } - - bb7: { ++ bb4: { _0 = const 1_i32; - drop(_7) -> [return: bb18, unwind: bb25]; + drop(_7) -> [return: bb15, unwind: bb22]; @@ -183,7 +184,7 @@ StorageDead(_12); StorageDead(_8); StorageDead(_6); -- falseEdge -> [real: bb2, imaginary: bb4]; +- falseEdge -> [real: bb2, imaginary: bb3]; + goto -> bb2; } diff --git a/tests/mir-opt/match_arm_scopes.complicated_match.panic-unwind.SimplifyCfg-initial.after-ElaborateDrops.after.diff b/tests/mir-opt/match_arm_scopes.complicated_match.panic-unwind.SimplifyCfg-initial.after-ElaborateDrops.after.diff index 619fda339a6a9..307f7105dd2f1 100644 --- a/tests/mir-opt/match_arm_scopes.complicated_match.panic-unwind.SimplifyCfg-initial.after-ElaborateDrops.after.diff +++ b/tests/mir-opt/match_arm_scopes.complicated_match.panic-unwind.SimplifyCfg-initial.after-ElaborateDrops.after.diff @@ -42,11 +42,15 @@ } bb2: { -- switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb4]; +- switchInt((_2.0: bool)) -> [0: bb4, otherwise: bb3]; + switchInt((_2.0: bool)) -> [0: bb3, otherwise: bb17]; } bb3: { +- falseEdge -> [real: bb20, imaginary: bb4]; +- } +- +- bb4: { StorageLive(_15); _15 = (_2.1: bool); StorageLive(_16); @@ -55,12 +59,8 @@ + goto -> bb16; } - bb4: { -- falseEdge -> [real: bb20, imaginary: bb3]; -- } -- - bb5: { -- falseEdge -> [real: bb13, imaginary: bb4]; +- falseEdge -> [real: bb13, imaginary: bb3]; - } - - bb6: { @@ -68,6 +68,7 @@ - } - - bb7: { ++ bb4: { _0 = const 1_i32; - drop(_7) -> [return: bb18, unwind: bb25]; + drop(_7) -> [return: bb15, unwind: bb22]; @@ -183,7 +184,7 @@ StorageDead(_12); StorageDead(_8); StorageDead(_6); -- falseEdge -> [real: bb2, imaginary: bb4]; +- falseEdge -> [real: bb2, imaginary: bb3]; + goto -> bb2; } From 8c3688cbb56b8fcb087261215a9215c001d4ea9d Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sat, 2 Mar 2024 02:14:13 +0100 Subject: [PATCH 3/5] Allocate candidate vectors as we sort them --- .../rustc_mir_build/src/build/matches/mod.rs | 46 +++++++++---------- .../rustc_mir_build/src/build/matches/test.rs | 36 +-------------- .../building/issue_49232.main.built.after.mir | 8 ++-- ...se_edges.full_tested_match.built.after.mir | 14 +++--- ...e_edges.full_tested_match2.built.after.mir | 22 ++++----- ...wise_branch.opt2.EarlyOtherwiseBranch.diff | 6 +-- ...nch_noopt.noopt1.EarlyOtherwiseBranch.diff | 12 ++--- 7 files changed, 56 insertions(+), 88 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index aea52fc497f0d..a1f620e7bafca 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -1653,10 +1653,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { &'b mut [&'c mut Candidate<'pat, 'tcx>], FxIndexMap, Vec<&'b mut Candidate<'pat, 'tcx>>>, ) { - // For each of the N possible outcomes, create a (initially empty) vector of candidates. - // Those are the candidates that apply if the test has that particular outcome. - let mut target_candidates: FxIndexMap<_, Vec<&mut Candidate<'pat, 'tcx>>> = - test.targets().into_iter().map(|branch| (branch, Vec::new())).collect(); + // For each of the possible outcomes, collect vector of candidates that apply if the test + // has that particular outcome. + let mut target_candidates: FxIndexMap<_, Vec<&mut Candidate<'_, '_>>> = Default::default(); let total_candidate_count = candidates.len(); @@ -1668,7 +1667,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { break; }; let (candidate, rest) = candidates.split_first_mut().unwrap(); - target_candidates[&branch].push(candidate); + target_candidates.entry(branch).or_insert_with(Vec::new).push(candidate); candidates = rest; } @@ -1809,31 +1808,32 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { otherwise_block }; - // 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. + // For each outcome of test, process the candidates that still apply. let target_blocks: FxIndexMap<_, _> = target_candidates .into_iter() .map(|(branch, 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, - ); - (branch, candidate_start) - } else { - (branch, remainder_start) - } + let candidate_start = self.cfg.start_new_block(); + self.match_candidates( + span, + scrutinee_span, + candidate_start, + remainder_start, + &mut *candidates, + ); + (branch, candidate_start) }) .collect(); // Perform the test, branching to one of N blocks. - self.perform_test(span, scrutinee_span, start_block, &match_place, &test, target_blocks); + self.perform_test( + span, + scrutinee_span, + start_block, + remainder_start, + &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 d003ae8d803ce..4244eb45045e4 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -127,6 +127,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { match_start_span: Span, scrutinee_span: Span, block: BasicBlock, + otherwise_block: BasicBlock, place_builder: &PlaceBuilder<'tcx>, test: &Test<'tcx>, target_blocks: FxIndexMap, BasicBlock>, @@ -134,14 +135,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let place = place_builder.to_place(self); let place_ty = place.ty(&self.local_decls, self.tcx); debug!(?place, ?place_ty); - let target_block = |branch| target_blocks[&branch]; + let target_block = |branch| target_blocks.get(&branch).copied().unwrap_or(otherwise_block); let source_info = self.source_info(test.span); match test.kind { TestKind::Switch { adt_def, ref variants } => { // 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); let otherwise_block = target_block(TestBranch::Failure); let tcx = self.tcx; let switch_targets = SwitchTargets::new( @@ -185,7 +185,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { TestKind::SwitchInt { ref options } => { // The switch may be inexhaustive so we have a catch-all block - debug_assert_eq!(options.len() + 1, target_blocks.len()); let otherwise_block = target_block(TestBranch::Failure); let switch_targets = SwitchTargets::new( options @@ -201,7 +200,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } TestKind::If => { - debug_assert_eq!(target_blocks.len(), 2); let success_block = target_block(TestBranch::Success); let fail_block = target_block(TestBranch::Failure); let terminator = @@ -211,7 +209,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { TestKind::Eq { value, ty } => { let tcx = self.tcx; - debug_assert_eq!(target_blocks.len(), 2); let success_block = target_block(TestBranch::Success); let fail_block = target_block(TestBranch::Failure); if let ty::Adt(def, _) = ty.kind() @@ -290,7 +287,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } TestKind::Range(ref range) => { - debug_assert_eq!(target_blocks.len(), 2); let success = target_block(TestBranch::Success); let fail = target_block(TestBranch::Failure); // Test `val` by computing `lo <= val && val <= hi`, using primitive comparisons. @@ -337,7 +333,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // expected = let expected = self.push_usize(block, source_info, len); - debug_assert_eq!(target_blocks.len(), 2); let success_block = target_block(TestBranch::Success); let fail_block = target_block(TestBranch::Failure); // result = actual == expected OR result = actual < expected @@ -753,33 +748,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } } -impl<'tcx> Test<'tcx> { - pub(super) fn targets(&self) -> Vec> { - match self.kind { - TestKind::Eq { .. } | TestKind::Range(_) | TestKind::Len { .. } | TestKind::If => { - vec![TestBranch::Success, TestBranch::Failure] - } - TestKind::Switch { adt_def, .. } => { - // While the switch that we generate doesn't test for all - // variants, we have a target for each variant and the - // otherwise case, and we make sure that all of the cases not - // specified have the same block. - adt_def - .variants() - .indices() - .map(|idx| TestBranch::Variant(idx)) - .chain([TestBranch::Failure]) - .collect() - } - TestKind::SwitchInt { ref options } => options - .iter() - .map(|(val, bits)| TestBranch::Constant(*val, *bits)) - .chain([TestBranch::Failure]) - .collect(), - } - } -} - fn is_switch_ty(ty: Ty<'_>) -> bool { ty.is_integral() || ty.is_char() } diff --git a/tests/mir-opt/building/issue_49232.main.built.after.mir b/tests/mir-opt/building/issue_49232.main.built.after.mir index 166e28ce51d42..d09a1748a8b36 100644 --- a/tests/mir-opt/building/issue_49232.main.built.after.mir +++ b/tests/mir-opt/building/issue_49232.main.built.after.mir @@ -25,7 +25,7 @@ fn main() -> () { StorageLive(_3); _3 = const true; PlaceMention(_3); - switchInt(_3) -> [0: bb6, otherwise: bb4]; + switchInt(_3) -> [0: bb4, otherwise: bb6]; } bb3: { @@ -34,8 +34,7 @@ fn main() -> () { } bb4: { - _0 = const (); - goto -> bb13; + falseEdge -> [real: bb8, imaginary: bb6]; } bb5: { @@ -43,7 +42,8 @@ fn main() -> () { } bb6: { - falseEdge -> [real: bb8, imaginary: bb4]; + _0 = const (); + goto -> bb13; } bb7: { diff --git a/tests/mir-opt/building/match_false_edges.full_tested_match.built.after.mir b/tests/mir-opt/building/match_false_edges.full_tested_match.built.after.mir index 4e91eb6f76fc4..194afdf7dd8a8 100644 --- a/tests/mir-opt/building/match_false_edges.full_tested_match.built.after.mir +++ b/tests/mir-opt/building/match_false_edges.full_tested_match.built.after.mir @@ -28,7 +28,7 @@ fn full_tested_match() -> () { _2 = Option::::Some(const 42_i32); PlaceMention(_2); _3 = discriminant(_2); - switchInt(move _3) -> [0: bb2, 1: bb4, otherwise: bb1]; + switchInt(move _3) -> [0: bb5, 1: bb2, otherwise: bb1]; } bb1: { @@ -37,20 +37,20 @@ fn full_tested_match() -> () { } bb2: { - _1 = (const 3_i32, const 3_i32); - goto -> bb13; + falseEdge -> [real: bb7, imaginary: bb3]; } bb3: { - goto -> bb1; + falseEdge -> [real: bb12, imaginary: bb5]; } bb4: { - falseEdge -> [real: bb7, imaginary: bb5]; + goto -> bb1; } bb5: { - falseEdge -> [real: bb12, imaginary: bb2]; + _1 = (const 3_i32, const 3_i32); + goto -> bb13; } bb6: { @@ -91,7 +91,7 @@ fn full_tested_match() -> () { bb11: { StorageDead(_7); StorageDead(_6); - goto -> bb5; + goto -> bb3; } bb12: { diff --git a/tests/mir-opt/building/match_false_edges.full_tested_match2.built.after.mir b/tests/mir-opt/building/match_false_edges.full_tested_match2.built.after.mir index 0c67cc9f71e53..ae83075434f7b 100644 --- a/tests/mir-opt/building/match_false_edges.full_tested_match2.built.after.mir +++ b/tests/mir-opt/building/match_false_edges.full_tested_match2.built.after.mir @@ -28,7 +28,7 @@ fn full_tested_match2() -> () { _2 = Option::::Some(const 42_i32); PlaceMention(_2); _3 = discriminant(_2); - switchInt(move _3) -> [0: bb2, 1: bb4, otherwise: bb1]; + switchInt(move _3) -> [0: bb5, 1: bb2, otherwise: bb1]; } bb1: { @@ -37,18 +37,10 @@ fn full_tested_match2() -> () { } bb2: { - falseEdge -> [real: bb12, imaginary: bb5]; + falseEdge -> [real: bb7, imaginary: bb5]; } bb3: { - goto -> bb1; - } - - bb4: { - falseEdge -> [real: bb7, imaginary: bb2]; - } - - bb5: { StorageLive(_9); _9 = ((_2 as Some).0: i32); StorageLive(_10); @@ -59,6 +51,14 @@ fn full_tested_match2() -> () { goto -> bb13; } + bb4: { + goto -> bb1; + } + + bb5: { + falseEdge -> [real: bb12, imaginary: bb3]; + } + bb6: { goto -> bb1; } @@ -97,7 +97,7 @@ fn full_tested_match2() -> () { bb11: { StorageDead(_7); StorageDead(_6); - falseEdge -> [real: bb5, imaginary: bb2]; + falseEdge -> [real: bb3, imaginary: bb5]; } bb12: { diff --git a/tests/mir-opt/early_otherwise_branch.opt2.EarlyOtherwiseBranch.diff b/tests/mir-opt/early_otherwise_branch.opt2.EarlyOtherwiseBranch.diff index 32a8dd8b8b4fb..0aed59be79446 100644 --- a/tests/mir-opt/early_otherwise_branch.opt2.EarlyOtherwiseBranch.diff +++ b/tests/mir-opt/early_otherwise_branch.opt2.EarlyOtherwiseBranch.diff @@ -30,7 +30,7 @@ StorageDead(_5); StorageDead(_4); _8 = discriminant((_3.0: std::option::Option)); -- switchInt(move _8) -> [0: bb2, 1: bb3, otherwise: bb1]; +- switchInt(move _8) -> [0: bb3, 1: bb2, otherwise: bb1]; + StorageLive(_11); + _11 = discriminant((_3.1: std::option::Option)); + StorageLive(_12); @@ -48,12 +48,12 @@ bb2: { - _6 = discriminant((_3.1: std::option::Option)); -- switchInt(move _6) -> [0: bb5, otherwise: bb1]; +- switchInt(move _6) -> [1: bb4, otherwise: bb1]; - } - - bb3: { - _7 = discriminant((_3.1: std::option::Option)); -- switchInt(move _7) -> [1: bb4, otherwise: bb1]; +- switchInt(move _7) -> [0: bb5, otherwise: bb1]; - } - - bb4: { diff --git a/tests/mir-opt/early_otherwise_branch_noopt.noopt1.EarlyOtherwiseBranch.diff b/tests/mir-opt/early_otherwise_branch_noopt.noopt1.EarlyOtherwiseBranch.diff index d7908ab3cd2ad..d446a5003a39b 100644 --- a/tests/mir-opt/early_otherwise_branch_noopt.noopt1.EarlyOtherwiseBranch.diff +++ b/tests/mir-opt/early_otherwise_branch_noopt.noopt1.EarlyOtherwiseBranch.diff @@ -36,7 +36,7 @@ StorageDead(_5); StorageDead(_4); _8 = discriminant((_3.0: std::option::Option)); - switchInt(move _8) -> [0: bb2, 1: bb4, otherwise: bb1]; + switchInt(move _8) -> [0: bb3, 1: bb2, otherwise: bb1]; } bb1: { @@ -45,17 +45,17 @@ bb2: { _6 = discriminant((_3.1: std::option::Option)); - switchInt(move _6) -> [0: bb3, 1: bb7, otherwise: bb1]; + switchInt(move _6) -> [0: bb6, 1: bb5, otherwise: bb1]; } bb3: { - _0 = const 3_u32; - goto -> bb8; + _7 = discriminant((_3.1: std::option::Option)); + switchInt(move _7) -> [0: bb4, 1: bb7, otherwise: bb1]; } bb4: { - _7 = discriminant((_3.1: std::option::Option)); - switchInt(move _7) -> [0: bb6, 1: bb5, otherwise: bb1]; + _0 = const 3_u32; + goto -> bb8; } bb5: { From edea739292f6ca2c69ad0d70d250806b579a1172 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Fri, 1 Mar 2024 01:59:04 +0100 Subject: [PATCH 4/5] No need to collect test variants ahead of time --- .../rustc_mir_build/src/build/matches/mod.rs | 45 ++---- .../rustc_mir_build/src/build/matches/test.rs | 140 ++++-------------- 2 files changed, 38 insertions(+), 147 deletions(-) diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index a1f620e7bafca..fdd41eeabecc0 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -14,7 +14,6 @@ use rustc_data_structures::{ fx::{FxHashSet, FxIndexMap, FxIndexSet}, stack::ensure_sufficient_stack, }; -use rustc_index::bit_set::BitSet; use rustc_middle::middle::region; use rustc_middle::mir::{self, *}; use rustc_middle::thir::{self, *}; @@ -1116,19 +1115,10 @@ enum TestKind<'tcx> { Switch { /// The enum type being tested. adt_def: ty::AdtDef<'tcx>, - /// The set of variants that we should create a branch for. We also - /// create an additional "otherwise" case. - variants: BitSet, }, /// Test what value an integer or `char` has. - SwitchInt { - /// The (ordered) set of values that we test for. - /// - /// We create a branch to each of the values in `options`, as well as an "otherwise" branch - /// for all other values, even in the (rare) case that `options` is exhaustive. - options: FxIndexMap, u128>, - }, + SwitchInt, /// Test what value a `bool` has. If, @@ -1173,6 +1163,12 @@ enum TestBranch<'tcx> { Failure, } +impl<'tcx> TestBranch<'tcx> { + fn as_constant(&self) -> Option<&Const<'tcx>> { + if let Self::Constant(v, _) = self { Some(v) } else { None } + } +} + /// `ArmHasGuard` is a wrapper around a boolean flag. It indicates whether /// a match arm has a guard expression attached to it. #[derive(Copy, Clone, Debug)] @@ -1583,30 +1579,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ) -> (PlaceBuilder<'tcx>, Test<'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); + let test = self.test(match_pair); let match_place = match_pair.place.clone(); - debug!(?test, ?match_pair); - // Most of the time, the test to perform is simply a function of the main candidate; but for - // a test like SwitchInt, we may want to add cases based on the candidates that are - // available - match test.kind { - TestKind::SwitchInt { ref mut options } => { - for candidate in candidates.iter() { - if !self.add_cases_to_switch(&match_place, candidate, options) { - break; - } - } - } - TestKind::Switch { adt_def: _, ref mut variants } => { - for candidate in candidates.iter() { - if !self.add_variants_to_switch(&match_place, candidate, variants) { - break; - } - } - } - _ => {} - } (match_place, test) } @@ -1663,7 +1638,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // point we may encounter a candidate where the test is not relevant; at that point, we stop // sorting. while let Some(candidate) = candidates.first_mut() { - let Some(branch) = self.sort_candidate(&match_place, &test, candidate) else { + let Some(branch) = + self.sort_candidate(&match_place, test, candidate, &target_candidates) + else { break; }; let (candidate, rest) = candidates.split_first_mut().unwrap(); diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index 4244eb45045e4..0598ffccea7b7 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -10,9 +10,7 @@ use crate::build::matches::{Candidate, MatchPair, Test, TestBranch, TestCase, Te use crate::build::Builder; use rustc_data_structures::fx::FxIndexMap; use rustc_hir::{LangItem, RangeEnd}; -use rustc_index::bit_set::BitSet; use rustc_middle::mir::*; -use rustc_middle::thir::*; use rustc_middle::ty::util::IntTypeExt; use rustc_middle::ty::GenericArg; use rustc_middle::ty::{self, adjustment::PointerCoercion, Ty, TyCtxt}; @@ -20,7 +18,6 @@ use rustc_span::def_id::DefId; use rustc_span::source_map::Spanned; use rustc_span::symbol::{sym, Symbol}; use rustc_span::{Span, DUMMY_SP}; -use rustc_target::abi::VariantIdx; use std::cmp::Ordering; @@ -30,22 +27,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// It is a bug to call this with a not-fully-simplified pattern. pub(super) fn test<'pat>(&mut self, match_pair: &MatchPair<'pat, 'tcx>) -> Test<'tcx> { let kind = match match_pair.test_case { - TestCase::Variant { adt_def, variant_index: _ } => { - TestKind::Switch { adt_def, variants: BitSet::new_empty(adt_def.variants().len()) } - } + TestCase::Variant { adt_def, variant_index: _ } => TestKind::Switch { adt_def }, TestCase::Constant { .. } if match_pair.pattern.ty.is_bool() => TestKind::If, - - TestCase::Constant { .. } if is_switch_ty(match_pair.pattern.ty) => { - // For integers, we use a `SwitchInt` match, which allows - // us to handle more cases. - TestKind::SwitchInt { - // these maps are empty to start; cases are - // added below in add_cases_to_switch - options: Default::default(), - } - } - + TestCase::Constant { .. } if is_switch_ty(match_pair.pattern.ty) => TestKind::SwitchInt, TestCase::Constant { value } => TestKind::Eq { value, ty: match_pair.pattern.ty }, TestCase::Range(range) => { @@ -70,57 +55,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { Test { span: match_pair.pattern.span, kind } } - pub(super) fn add_cases_to_switch<'pat>( - &mut self, - test_place: &PlaceBuilder<'tcx>, - candidate: &Candidate<'pat, 'tcx>, - options: &mut FxIndexMap, u128>, - ) -> bool { - let Some(match_pair) = candidate.match_pairs.iter().find(|mp| mp.place == *test_place) - else { - return false; - }; - - match match_pair.test_case { - TestCase::Constant { value } => { - options.entry(value).or_insert_with(|| value.eval_bits(self.tcx, self.param_env)); - true - } - TestCase::Variant { .. } => { - panic!("you should have called add_variants_to_switch instead!"); - } - TestCase::Range(ref range) => { - // Check that none of the switch values are in the range. - self.values_not_contained_in_range(&*range, options).unwrap_or(false) - } - // don't know how to add these patterns to a switch - _ => false, - } - } - - pub(super) fn add_variants_to_switch<'pat>( - &mut self, - test_place: &PlaceBuilder<'tcx>, - candidate: &Candidate<'pat, 'tcx>, - variants: &mut BitSet, - ) -> bool { - let Some(match_pair) = candidate.match_pairs.iter().find(|mp| mp.place == *test_place) - else { - return false; - }; - - match match_pair.test_case { - TestCase::Variant { variant_index, .. } => { - // We have a pattern testing for variant `variant_index` - // set the corresponding index to true - variants.insert(variant_index); - true - } - // don't know how to add these patterns to a switch - _ => false, - } - } - #[instrument(skip(self, target_blocks, place_builder), level = "debug")] pub(super) fn perform_test( &mut self, @@ -139,33 +73,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let source_info = self.source_info(test.span); match test.kind { - TestKind::Switch { adt_def, ref variants } => { - // Variants is a BitVec of indexes into adt_def.variants. - let num_enum_variants = adt_def.variants().len(); + TestKind::Switch { adt_def } => { let otherwise_block = target_block(TestBranch::Failure); - let tcx = self.tcx; let switch_targets = SwitchTargets::new( - adt_def.discriminants(tcx).filter_map(|(idx, discr)| { - if variants.contains(idx) { - debug_assert_ne!( - target_block(TestBranch::Variant(idx)), - otherwise_block, - "no candidates for tested discriminant: {discr:?}", - ); - Some((discr.val, target_block(TestBranch::Variant(idx)))) + adt_def.discriminants(self.tcx).filter_map(|(idx, discr)| { + if let Some(&block) = target_blocks.get(&TestBranch::Variant(idx)) { + Some((discr.val, block)) } else { - debug_assert_eq!( - target_block(TestBranch::Variant(idx)), - otherwise_block, - "found candidates for untested discriminant: {discr:?}", - ); None } }), otherwise_block, ); - debug!("num_enum_variants: {}, variants: {:?}", num_enum_variants, variants); - let discr_ty = adt_def.repr().discr_type().to_ty(tcx); + debug!("num_enum_variants: {}", adt_def.variants().len()); + let discr_ty = adt_def.repr().discr_type().to_ty(self.tcx); let discr = self.temp(discr_ty, test.span); self.cfg.push_assign( block, @@ -183,13 +104,17 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ); } - TestKind::SwitchInt { ref options } => { + TestKind::SwitchInt => { // The switch may be inexhaustive so we have a catch-all block let otherwise_block = target_block(TestBranch::Failure); let switch_targets = SwitchTargets::new( - options - .iter() - .map(|(&val, &bits)| (bits, target_block(TestBranch::Constant(val, bits)))), + target_blocks.iter().filter_map(|(&branch, &block)| { + if let TestBranch::Constant(_, bits) = branch { + Some((bits, block)) + } else { + None + } + }), otherwise_block, ); let terminator = TerminatorKind::SwitchInt { @@ -548,11 +473,12 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { /// that it *doesn't* apply. For now, we return false, indicate that the /// test does not apply to this candidate, but it might be we can get /// tighter match code if we do something a bit different. - pub(super) fn sort_candidate<'pat>( + pub(super) fn sort_candidate( &mut self, test_place: &PlaceBuilder<'tcx>, test: &Test<'tcx>, - candidate: &mut Candidate<'pat, 'tcx>, + candidate: &mut Candidate<'_, 'tcx>, + sorted_candidates: &FxIndexMap, Vec<&mut Candidate<'_, 'tcx>>>, ) -> Option> { // Find the match_pair for this place (if any). At present, // afaik, there can be at most one. (In the future, if we @@ -568,7 +494,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // If we are performing a variant switch, then this // informs variant patterns, but nothing else. ( - &TestKind::Switch { adt_def: tested_adt_def, .. }, + &TestKind::Switch { adt_def: tested_adt_def }, &TestCase::Variant { adt_def, variant_index }, ) => { assert_eq!(adt_def, tested_adt_def); @@ -581,17 +507,19 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { // // FIXME(#29623) we could use PatKind::Range to rule // things out here, in some cases. - (TestKind::SwitchInt { options }, &TestCase::Constant { value }) + (TestKind::SwitchInt, &TestCase::Constant { value }) if is_switch_ty(match_pair.pattern.ty) => { fully_matched = true; - let bits = options.get(&value).unwrap(); - Some(TestBranch::Constant(value, *bits)) + let bits = value.eval_bits(self.tcx, self.param_env); + Some(TestBranch::Constant(value, bits)) } - (TestKind::SwitchInt { options }, TestCase::Range(range)) => { + (TestKind::SwitchInt, TestCase::Range(range)) => { fully_matched = false; let not_contained = - self.values_not_contained_in_range(&*range, options).unwrap_or(false); + sorted_candidates.keys().filter_map(|br| br.as_constant()).copied().all( + |val| matches!(range.contains(val, self.tcx, self.param_env), Some(false)), + ); not_contained.then(|| { // No switch values are contained in the pattern range, @@ -732,20 +660,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { ret } - - fn values_not_contained_in_range( - &self, - range: &PatRange<'tcx>, - options: &FxIndexMap, u128>, - ) -> Option { - for &val in options.keys() { - if range.contains(val, self.tcx, self.param_env)? { - return Some(false); - } - } - - Some(true) - } } fn is_switch_ty(ty: Ty<'_>) -> bool { From d46ff6415c033ccfebac3d2a757908611a67d324 Mon Sep 17 00:00:00 2001 From: Nadrieril Date: Sat, 2 Mar 2024 02:49:33 +0100 Subject: [PATCH 5/5] Fix a subtle regression Before, the SwitchInt cases were computed in two passes: if the first pass accepted e.g. 0..=5 and then 1, the second pass would not accept 0..=5 anymore because 1 would be listed in the SwitchInt options. Now there's a single pass, so if we sort 0..=5 we must take care to not sort a subsequent 1. --- .../rustc_mir_build/src/build/matches/mod.rs | 6 +++++ .../rustc_mir_build/src/build/matches/test.rs | 27 ++++++++++++++++--- ...egression-switchint-sorting-with-ranges.rs | 14 ++++++++++ 3 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 tests/ui/pattern/usefulness/integer-ranges/regression-switchint-sorting-with-ranges.rs diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs index fdd41eeabecc0..cfd12cec0e468 100644 --- a/compiler/rustc_mir_build/src/build/matches/mod.rs +++ b/compiler/rustc_mir_build/src/build/matches/mod.rs @@ -1091,6 +1091,12 @@ enum TestCase<'pat, 'tcx> { Or { pats: Box<[FlatPat<'pat, 'tcx>]> }, } +impl<'pat, 'tcx> TestCase<'pat, 'tcx> { + fn as_range(&self) -> Option<&'pat PatRange<'tcx>> { + if let Self::Range(v) = self { Some(*v) } else { None } + } +} + #[derive(Debug, Clone)] pub(crate) struct MatchPair<'pat, 'tcx> { /// This place... diff --git a/compiler/rustc_mir_build/src/build/matches/test.rs b/compiler/rustc_mir_build/src/build/matches/test.rs index 0598ffccea7b7..9d77bd063e114 100644 --- a/compiler/rustc_mir_build/src/build/matches/test.rs +++ b/compiler/rustc_mir_build/src/build/matches/test.rs @@ -510,9 +510,30 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { (TestKind::SwitchInt, &TestCase::Constant { value }) if is_switch_ty(match_pair.pattern.ty) => { - fully_matched = true; - let bits = value.eval_bits(self.tcx, self.param_env); - Some(TestBranch::Constant(value, bits)) + // Beware: there might be some ranges sorted into the failure case; we must not add + // a success case that could be matched by one of these ranges. + let is_covering_range = |test_case: &TestCase<'_, 'tcx>| { + test_case.as_range().is_some_and(|range| { + matches!(range.contains(value, self.tcx, self.param_env), None | Some(true)) + }) + }; + let is_conflicting_candidate = |candidate: &&mut Candidate<'_, 'tcx>| { + candidate + .match_pairs + .iter() + .any(|mp| mp.place == *test_place && is_covering_range(&mp.test_case)) + }; + if sorted_candidates + .get(&TestBranch::Failure) + .is_some_and(|candidates| candidates.iter().any(is_conflicting_candidate)) + { + fully_matched = false; + None + } else { + fully_matched = true; + let bits = value.eval_bits(self.tcx, self.param_env); + Some(TestBranch::Constant(value, bits)) + } } (TestKind::SwitchInt, TestCase::Range(range)) => { fully_matched = false; diff --git a/tests/ui/pattern/usefulness/integer-ranges/regression-switchint-sorting-with-ranges.rs b/tests/ui/pattern/usefulness/integer-ranges/regression-switchint-sorting-with-ranges.rs new file mode 100644 index 0000000000000..bacb60a108bfc --- /dev/null +++ b/tests/ui/pattern/usefulness/integer-ranges/regression-switchint-sorting-with-ranges.rs @@ -0,0 +1,14 @@ +//@ run-pass +// +// Regression test for match lowering to MIR: when gathering candidates, by the time we get to the +// range we know the range will only match on the failure case of the switchint. Hence we mustn't +// add the `1` to the switchint or the range would be incorrectly sorted. +#![allow(unreachable_patterns)] +fn main() { + match 1 { + 10 => unreachable!(), + 0..=5 => {} + 1 => unreachable!(), + _ => unreachable!(), + } +}