From 36e0c36652170e6d04d85e4edbbfdb886aae1de4 Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Sat, 2 Mar 2024 19:47:16 +0100
Subject: [PATCH 1/9] Add tests
---
tests/ui/or-patterns/bindings-runpass-2.rs | 1 +
tests/ui/or-patterns/inner-or-pat.or3.stderr | 2 +-
tests/ui/or-patterns/inner-or-pat.or4.stderr | 2 +-
tests/ui/or-patterns/inner-or-pat.rs | 4 +---
...ssue-70413-no-unreachable-pat-and-guard.rs | 21 +++++++++---------
tests/ui/or-patterns/search-via-bindings.rs | 22 +++++++++++++++++++
6 files changed, 36 insertions(+), 16 deletions(-)
diff --git a/tests/ui/or-patterns/bindings-runpass-2.rs b/tests/ui/or-patterns/bindings-runpass-2.rs
index 657d7f1ed189f..a9ae998108405 100644
--- a/tests/ui/or-patterns/bindings-runpass-2.rs
+++ b/tests/ui/or-patterns/bindings-runpass-2.rs
@@ -26,5 +26,6 @@ fn main() {
assert_eq!(or_at(Err(7)), 207);
assert_eq!(or_at(Err(8)), 8);
assert_eq!(or_at(Err(20)), 220);
+ assert_eq!(or_at(Err(34)), 134);
assert_eq!(or_at(Err(50)), 500);
}
diff --git a/tests/ui/or-patterns/inner-or-pat.or3.stderr b/tests/ui/or-patterns/inner-or-pat.or3.stderr
index 10ec7c202e4d6..5c522a97ccef3 100644
--- a/tests/ui/or-patterns/inner-or-pat.or3.stderr
+++ b/tests/ui/or-patterns/inner-or-pat.or3.stderr
@@ -1,5 +1,5 @@
error[E0308]: mismatched types
- --> $DIR/inner-or-pat.rs:38:54
+ --> $DIR/inner-or-pat.rs:36:54
|
LL | match x {
| - this expression has type `&str`
diff --git a/tests/ui/or-patterns/inner-or-pat.or4.stderr b/tests/ui/or-patterns/inner-or-pat.or4.stderr
index 97800161d82fe..508520c823793 100644
--- a/tests/ui/or-patterns/inner-or-pat.or4.stderr
+++ b/tests/ui/or-patterns/inner-or-pat.or4.stderr
@@ -1,5 +1,5 @@
error[E0408]: variable `x` is not bound in all patterns
- --> $DIR/inner-or-pat.rs:53:37
+ --> $DIR/inner-or-pat.rs:51:37
|
LL | (x @ "red" | (x @ "blue" | "red")) => {
| - ^^^^^ pattern doesn't bind `x`
diff --git a/tests/ui/or-patterns/inner-or-pat.rs b/tests/ui/or-patterns/inner-or-pat.rs
index ceb0a8b3f7969..4d136de005357 100644
--- a/tests/ui/or-patterns/inner-or-pat.rs
+++ b/tests/ui/or-patterns/inner-or-pat.rs
@@ -1,7 +1,5 @@
-//@ revisions: or1 or2 or3 or4 or5
+//@ revisions: or1 or3 or4
//@ [or1] run-pass
-//@ [or2] run-pass
-//@ [or5] run-pass
#![allow(unreachable_patterns)]
#![allow(unused_variables)]
diff --git a/tests/ui/or-patterns/issue-70413-no-unreachable-pat-and-guard.rs b/tests/ui/or-patterns/issue-70413-no-unreachable-pat-and-guard.rs
index 7d62364a6aeec..76dc298a5c851 100644
--- a/tests/ui/or-patterns/issue-70413-no-unreachable-pat-and-guard.rs
+++ b/tests/ui/or-patterns/issue-70413-no-unreachable-pat-and-guard.rs
@@ -1,21 +1,20 @@
-//@ check-pass
+//@ run-pass
#![deny(unreachable_patterns)]
fn main() {
- match (3,42) {
- (a,_) | (_,a) if a > 10 => {println!("{}", a)}
- _ => ()
+ match (3, 42) {
+ (a, _) | (_, a) if a > 10 => {}
+ _ => unreachable!(),
}
- match Some((3,42)) {
- Some((a, _)) | Some((_, a)) if a > 10 => {println!("{}", a)}
- _ => ()
-
+ match Some((3, 42)) {
+ Some((a, _)) | Some((_, a)) if a > 10 => {}
+ _ => unreachable!(),
}
- match Some((3,42)) {
- Some((a, _) | (_, a)) if a > 10 => {println!("{}", a)}
- _ => ()
+ match Some((3, 42)) {
+ Some((a, _) | (_, a)) if a > 10 => {}
+ _ => unreachable!(),
}
}
diff --git a/tests/ui/or-patterns/search-via-bindings.rs b/tests/ui/or-patterns/search-via-bindings.rs
index a760112f1d421..42174bd7cef73 100644
--- a/tests/ui/or-patterns/search-via-bindings.rs
+++ b/tests/ui/or-patterns/search-via-bindings.rs
@@ -42,6 +42,23 @@ fn search_old_style(target: (bool, bool, bool)) -> u32 {
}
}
+// Check that a dummy or-pattern also leads to running the guard multiple times.
+fn search_with_dummy(target: (bool, bool)) -> u32 {
+ let x = ((false, true), (false, true), ());
+ let mut guard_count = 0;
+ match x {
+ ((a, _) | (_, a), (b, _) | (_, b), _ | _)
+ if {
+ guard_count += 1;
+ (a, b) == target
+ } =>
+ {
+ guard_count
+ }
+ _ => unreachable!(),
+ }
+}
+
fn main() {
assert_eq!(search((false, false, false)), 1);
assert_eq!(search((false, false, true)), 2);
@@ -60,4 +77,9 @@ fn main() {
assert_eq!(search_old_style((true, false, true)), 6);
assert_eq!(search_old_style((true, true, false)), 7);
assert_eq!(search_old_style((true, true, true)), 8);
+
+ assert_eq!(search_with_dummy((false, false)), 1);
+ assert_eq!(search_with_dummy((false, true)), 3);
+ assert_eq!(search_with_dummy((true, false)), 5);
+ assert_eq!(search_with_dummy((true, true)), 7);
}
From 1091425b8e5e18bfd26d112c54d2f3f9cf19f664 Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Sat, 2 Mar 2024 20:21:58 +0100
Subject: [PATCH 2/9] Tweak `test_candidates_with_or`
---
.../rustc_mir_build/src/build/matches/mod.rs | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 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..4bcd6c1761ead 100644
--- a/compiler/rustc_mir_build/src/build/matches/mod.rs
+++ b/compiler/rustc_mir_build/src/build/matches/mod.rs
@@ -1441,20 +1441,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
return;
}
- let match_pairs = mem::take(&mut first_candidate.match_pairs);
- let (first_match_pair, remaining_match_pairs) = match_pairs.split_first().unwrap();
- let TestCase::Or { ref pats } = &first_match_pair.test_case else { unreachable!() };
+ let first_match_pair = first_candidate.match_pairs.remove(0);
+ let or_span = first_match_pair.pattern.span;
+ let TestCase::Or { pats } = first_match_pair.test_case else { unreachable!() };
let remainder_start = self.cfg.start_new_block();
- let or_span = first_match_pair.pattern.span;
// Test the alternatives of this or-pattern.
self.test_or_pattern(first_candidate, start_block, remainder_start, pats, or_span);
- if !remaining_match_pairs.is_empty() {
+ if !first_candidate.match_pairs.is_empty() {
// If more match pairs remain, test them after each subcandidate.
// We could add them to the or-candidates before the call to `test_or_pattern` but this
// would make it impossible to detect simplifiable or-patterns. That would guarantee
// exponentially large CFGs for cases like `(1 | 2, 3 | 4, ...)`.
+ let remaining_match_pairs = mem::take(&mut first_candidate.match_pairs);
first_candidate.visit_leaves(|leaf_candidate| {
assert!(leaf_candidate.match_pairs.is_empty());
leaf_candidate.match_pairs.extend(remaining_match_pairs.iter().cloned());
@@ -1492,13 +1492,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
candidate: &mut Candidate<'pat, 'tcx>,
start_block: BasicBlock,
otherwise_block: BasicBlock,
- pats: &[FlatPat<'pat, 'tcx>],
+ pats: Box<[FlatPat<'pat, 'tcx>]>,
or_span: Span,
) {
debug!("candidate={:#?}\npats={:#?}", candidate, pats);
let mut or_candidates: Vec<_> = pats
- .iter()
- .cloned()
+ .into_vec()
+ .into_iter()
.map(|flat_pat| Candidate::from_flat_pat(flat_pat, candidate.has_guard))
.collect();
let mut or_candidate_refs: Vec<_> = or_candidates.iter_mut().collect();
From 0ad2eff26eca29dd1c7308d4e909a4aa0a525c3e Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Fri, 1 Mar 2024 23:36:49 +0100
Subject: [PATCH 3/9] Precompute the presence of bindings and ascriptions
---
.../rustc_mir_build/src/build/matches/mod.rs | 36 +++++++++++++++----
.../rustc_mir_build/src/build/matches/util.rs | 15 ++++++--
2 files changed, 41 insertions(+), 10 deletions(-)
diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs
index 4bcd6c1761ead..571467759279d 100644
--- a/compiler/rustc_mir_build/src/build/matches/mod.rs
+++ b/compiler/rustc_mir_build/src/build/matches/mod.rs
@@ -954,6 +954,9 @@ struct FlatPat<'pat, 'tcx> {
/// ...and these types asserted.
ascriptions: Vec<Ascription<'tcx>>,
+
+ /// Whether this recursively contains no bindings or ascriptions.
+ simple: bool,
}
impl<'tcx, 'pat> FlatPat<'pat, 'tcx> {
@@ -968,7 +971,11 @@ impl<'tcx, 'pat> FlatPat<'pat, 'tcx> {
cx.simplify_match_pairs(&mut match_pairs, &mut bindings, &mut ascriptions);
- FlatPat { span: pattern.span, match_pairs, bindings, ascriptions }
+ let simple = bindings.is_empty()
+ && ascriptions.is_empty()
+ && match_pairs.iter().all(|mp| mp.is_simple());
+
+ FlatPat { span: pattern.span, match_pairs, bindings, ascriptions, simple }
}
}
@@ -1084,12 +1091,27 @@ struct Ascription<'tcx> {
#[derive(Debug, Clone)]
enum TestCase<'pat, 'tcx> {
- Irrefutable { binding: Option<Binding<'tcx>>, ascription: Option<Ascription<'tcx>> },
- Variant { adt_def: ty::AdtDef<'tcx>, variant_index: VariantIdx },
- Constant { value: mir::Const<'tcx> },
+ Irrefutable {
+ binding: Option<Binding<'tcx>>,
+ ascription: Option<Ascription<'tcx>>,
+ },
+ Variant {
+ adt_def: ty::AdtDef<'tcx>,
+ variant_index: VariantIdx,
+ },
+ Constant {
+ value: mir::Const<'tcx>,
+ },
Range(&'pat PatRange<'tcx>),
- Slice { len: usize, variable_length: bool },
- Or { pats: Box<[FlatPat<'pat, 'tcx>]> },
+ Slice {
+ len: usize,
+ variable_length: bool,
+ },
+ Or {
+ pats: Box<[FlatPat<'pat, 'tcx>]>,
+ /// Whether this recursively contains no bindings or ascriptions.
+ simple: bool,
+ },
}
#[derive(Debug, Clone)]
@@ -1443,7 +1465,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let first_match_pair = first_candidate.match_pairs.remove(0);
let or_span = first_match_pair.pattern.span;
- let TestCase::Or { pats } = first_match_pair.test_case else { unreachable!() };
+ let TestCase::Or { pats, .. } = first_match_pair.test_case else { unreachable!() };
let remainder_start = self.cfg.start_new_block();
// Test the alternatives of this or-pattern.
diff --git a/compiler/rustc_mir_build/src/build/matches/util.rs b/compiler/rustc_mir_build/src/build/matches/util.rs
index 2351f69a9141f..c2ef47193230c 100644
--- a/compiler/rustc_mir_build/src/build/matches/util.rs
+++ b/compiler/rustc_mir_build/src/build/matches/util.rs
@@ -122,9 +122,12 @@ impl<'pat, 'tcx> MatchPair<'pat, 'tcx> {
let mut subpairs = Vec::new();
let test_case = match pattern.kind {
PatKind::Never | PatKind::Wild | PatKind::Error(_) => default_irrefutable(),
- PatKind::Or { ref pats } => TestCase::Or {
- pats: pats.iter().map(|pat| FlatPat::new(place.clone(), pat, cx)).collect(),
- },
+ PatKind::Or { ref pats } => {
+ let pats: Box<[_]> =
+ pats.iter().map(|pat| FlatPat::new(place.clone(), pat, cx)).collect();
+ let simple = pats.iter().all(|fpat| fpat.simple);
+ TestCase::Or { pats, simple }
+ }
PatKind::Range(ref range) => {
if range.is_full_range(cx.tcx) == Some(true) {
@@ -260,6 +263,12 @@ impl<'pat, 'tcx> MatchPair<'pat, 'tcx> {
MatchPair { place, test_case, subpairs, pattern }
}
+
+ /// Whether this recursively contains no bindings or ascriptions.
+ pub(super) fn is_simple(&self) -> bool {
+ !matches!(self.test_case, TestCase::Or { simple: false, .. })
+ && self.subpairs.iter().all(|p| p.is_simple())
+ }
}
pub(super) struct FakeBorrowCollector<'a, 'b, 'tcx> {
From 22b8a7d5dbdfb8f653b8d8c044425ea16e640595 Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Sat, 2 Mar 2024 20:22:31 +0100
Subject: [PATCH 4/9] Improve or-pattern simplification
We can now tell ahead of time whether and or-pattern will be
simplifiable or not. We use this to clarify the possible code paths.
---
.../rustc_mir_build/src/build/matches/mod.rs | 173 +++++++++---------
1 file changed, 87 insertions(+), 86 deletions(-)
diff --git a/compiler/rustc_mir_build/src/build/matches/mod.rs b/compiler/rustc_mir_build/src/build/matches/mod.rs
index 571467759279d..911b82bf4699d 100644
--- a/compiler/rustc_mir_build/src/build/matches/mod.rs
+++ b/compiler/rustc_mir_build/src/build/matches/mod.rs
@@ -1398,8 +1398,30 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
/// Tests a candidate where there are only or-patterns left to test, or
/// forwards to [Builder::test_candidates].
///
- /// Given a pattern `(P | Q, R | S)` we (in principle) generate a CFG like
- /// so:
+ /// Given a pattern `(P | Q, R | S)` we would like to generate a CFG like so:
+ ///
+ /// ```text
+ /// ...
+ /// +---------------+------------+
+ /// | | |
+ /// [ P matches ] [ Q matches ] [ otherwise ]
+ /// | | |
+ /// +---------------+ |
+ /// | ...
+ /// [ match R, S ]
+ /// |
+ /// ...
+ /// ```
+ ///
+ /// In practice there are some complications:
+ ///
+ /// * If `P` or `Q` has bindings or type ascriptions, we must generate separate branches for
+ /// each case.
+ /// * If there's a guard, we must also keep branches separate as this changes how many times
+ /// the guard is run.
+ /// * If `P` succeeds and `R | S` fails, we know `(Q, R | S)` will fail too. So we could skip
+ /// testing of `Q` in that case. Because we can't distinguish pattern failure from guard
+ /// failure, we only do this optimization when there is no guard. We then get this:
///
/// ```text
/// [ start ]
@@ -1427,27 +1449,36 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
/// [ Success ] [ Failure ]
/// ```
///
- /// In practice there are some complications:
- ///
- /// * If there's a guard, then the otherwise branch of the first match on
- /// `R | S` goes to a test for whether `Q` matches, and the control flow
- /// doesn't merge into a single success block until after the guard is
- /// tested.
- /// * If neither `P` or `Q` has any bindings or type ascriptions and there
- /// isn't a match guard, then we create a smaller CFG like:
+ /// * If there's a guard, then the otherwise branch of the first match on `R | S` goes to a test
+ /// for whether `Q` matches, and the control flow doesn't merge into a single success block
+ /// until after the guard is tested. In other words, the branches are kept entirely separate:
///
/// ```text
- /// ...
- /// +---------------+------------+
- /// | | |
- /// [ P matches ] [ Q matches ] [ otherwise ]
- /// | | |
- /// +---------------+ |
- /// | ...
- /// [ match R, S ]
+ /// [ start ]
/// |
- /// ...
+ /// [ match P, Q ]
+ /// |
+ /// +---------------------------+-------------+------------------------------------+
+ /// | | | |
+ /// V | V V
+ /// [ P matches ] | [ Q matches ] [ otherwise ]
+ /// | | | |
+ /// V [ otherwise ] V |
+ /// [ match R, S ] ^ [ match R, S ] |
+ /// | | | |
+ /// +--------------+------------+ +--------------+------------+ |
+ /// | | | | | |
+ /// V V V V V |
+ /// [ R matches ] [ S matches ] [ R matches ] [ S matches ] [otherwise ] |
+ /// | | | | | |
+ /// +--------------+--------------------------+--------------+ | |
+ /// | | |
+ /// | +--------+
+ /// | |
+ /// V V
+ /// [ Success ] [ Failure ]
/// ```
+ ///
fn test_candidates_with_or(
&mut self,
span: Span,
@@ -1465,25 +1496,43 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let first_match_pair = first_candidate.match_pairs.remove(0);
let or_span = first_match_pair.pattern.span;
- let TestCase::Or { pats, .. } = first_match_pair.test_case else { unreachable!() };
+ let TestCase::Or { pats, simple } = first_match_pair.test_case else { unreachable!() };
+ debug!("candidate={:#?}\npats={:#?}", first_candidate, pats);
- let remainder_start = self.cfg.start_new_block();
// Test the alternatives of this or-pattern.
- self.test_or_pattern(first_candidate, start_block, remainder_start, pats, or_span);
+ let remainder_start = self.cfg.start_new_block();
+ first_candidate.subcandidates = pats
+ .into_vec()
+ .into_iter()
+ .map(|flat_pat| Candidate::from_flat_pat(flat_pat, first_candidate.has_guard))
+ .collect();
+ let mut or_candidate_refs: Vec<_> = first_candidate.subcandidates.iter_mut().collect();
+ self.match_candidates(
+ or_span,
+ or_span,
+ start_block,
+ remainder_start,
+ &mut or_candidate_refs,
+ );
+
+ // Whether we need to keep the or-pattern branches separate.
+ let keep_branches_separate = first_candidate.has_guard || !simple;
+ if !keep_branches_separate {
+ self.merge_subcandidates(first_candidate, self.source_info(or_span));
+ }
if !first_candidate.match_pairs.is_empty() {
- // If more match pairs remain, test them after each subcandidate.
- // We could add them to the or-candidates before the call to `test_or_pattern` but this
- // would make it impossible to detect simplifiable or-patterns. That would guarantee
- // exponentially large CFGs for cases like `(1 | 2, 3 | 4, ...)`.
+ // If more match pairs remain, test them after each subcandidate. If we merged them,
+ // then this will only test `first_candidate`.
let remaining_match_pairs = mem::take(&mut first_candidate.match_pairs);
first_candidate.visit_leaves(|leaf_candidate| {
assert!(leaf_candidate.match_pairs.is_empty());
leaf_candidate.match_pairs.extend(remaining_match_pairs.iter().cloned());
let or_start = leaf_candidate.pre_binding_block.unwrap();
- // In a case like `(a | b, c | d)`, if `a` succeeds and `c | d` fails, we know `(b,
- // c | d)` will fail too. If there is no guard, we skip testing of `b` by branching
- // directly to `remainder_start`. If there is a guard, we have to try `(b, c | d)`.
+ // In a case like `(P | Q, R | S)`, if `P` succeeds and `R | S` fails, we know `(Q,
+ // R | S)` will fail too. If there is no guard, we skip testing of `Q` by branching
+ // directly to `remainder_start`. If there is a guard, `or_otherwise` can be reached
+ // by guard failure as well, so we can't skip `Q`.
let or_otherwise = leaf_candidate.otherwise_block.unwrap_or(remainder_start);
self.test_candidates_with_or(
span,
@@ -1505,68 +1554,20 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
);
}
- #[instrument(
- skip(self, start_block, otherwise_block, or_span, candidate, pats),
- level = "debug"
- )]
- fn test_or_pattern<'pat>(
- &mut self,
- candidate: &mut Candidate<'pat, 'tcx>,
- start_block: BasicBlock,
- otherwise_block: BasicBlock,
- pats: Box<[FlatPat<'pat, 'tcx>]>,
- or_span: Span,
- ) {
- debug!("candidate={:#?}\npats={:#?}", candidate, pats);
- let mut or_candidates: Vec<_> = pats
- .into_vec()
- .into_iter()
- .map(|flat_pat| Candidate::from_flat_pat(flat_pat, candidate.has_guard))
- .collect();
- let mut or_candidate_refs: Vec<_> = or_candidates.iter_mut().collect();
- self.match_candidates(
- or_span,
- or_span,
- start_block,
- otherwise_block,
- &mut or_candidate_refs,
- );
- candidate.subcandidates = or_candidates;
- self.merge_trivial_subcandidates(candidate, self.source_info(or_span));
- }
-
- /// Try to merge all of the subcandidates of the given candidate into one.
+ /// Merge all of the subcandidates of the given candidate into one.
/// This avoids exponentially large CFGs in cases like `(1 | 2, 3 | 4, ...)`.
- fn merge_trivial_subcandidates(
+ fn merge_subcandidates(
&mut self,
candidate: &mut Candidate<'_, 'tcx>,
source_info: SourceInfo,
) {
- if candidate.subcandidates.is_empty() || candidate.has_guard {
- // FIXME(or_patterns; matthewjasper) Don't give up if we have a guard.
- return;
- }
-
- let mut can_merge = true;
-
- // Not `Iterator::all` because we don't want to short-circuit.
- for subcandidate in &mut candidate.subcandidates {
- self.merge_trivial_subcandidates(subcandidate, source_info);
-
- // FIXME(or_patterns; matthewjasper) Try to be more aggressive here.
- can_merge &= subcandidate.subcandidates.is_empty()
- && subcandidate.bindings.is_empty()
- && subcandidate.ascriptions.is_empty();
- }
-
- if can_merge {
- let any_matches = self.cfg.start_new_block();
- for subcandidate in mem::take(&mut candidate.subcandidates) {
- let or_block = subcandidate.pre_binding_block.unwrap();
- self.cfg.goto(or_block, source_info, any_matches);
- }
- candidate.pre_binding_block = Some(any_matches);
- }
+ let any_matches = self.cfg.start_new_block();
+ candidate.visit_leaves(|leaf_candidate| {
+ let or_block = leaf_candidate.pre_binding_block.unwrap();
+ self.cfg.goto(or_block, source_info, any_matches);
+ });
+ candidate.subcandidates.clear();
+ candidate.pre_binding_block = Some(any_matches);
}
/// Pick a test to run. Which test doesn't matter as long as it is guaranteed to fully match at
From c2564d017aaf31d4afb66344883649ea95f8ea1b Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Fri, 1 Mar 2024 19:07:08 +0100
Subject: [PATCH 5/9] 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 0c9b5e2bac577b7be5acd27e63a146f47769ba5f Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Thu, 29 Feb 2024 00:52:03 +0100
Subject: [PATCH 6/9] 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 911b82bf4699d..150e7f790e915 100644
--- a/compiler/rustc_mir_build/src/build/matches/mod.rs
+++ b/compiler/rustc_mir_build/src/build/matches/mod.rs
@@ -1182,6 +1182,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)]
@@ -1659,11 +1672,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<Vec<&'b mut Candidate<'pat, 'tcx>>>) {
+ ) -> (
+ &'b mut [&'c mut Candidate<'pat, 'tcx>],
+ FxIndexMap<TestBranch<'tcx>, 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<&mut Candidate<'pat, 'tcx>>> = 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();
@@ -1671,11 +1687,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;
}
@@ -1820,9 +1836,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(
@@ -1832,9 +1848,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<BasicBlock>,
+ target_blocks: FxIndexMap<TestBranch<'tcx>, 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 = <N>
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<usize> {
+ ) -> Option<TestBranch<'tcx>> {
// 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<TestBranch<'tcx>> {
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 b3dc088aa72317472921eed7ccd2de6691114aa4 Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Sat, 2 Mar 2024 02:14:13 +0100
Subject: [PATCH 7/9] 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 150e7f790e915..aeadd6c770318 100644
--- a/compiler/rustc_mir_build/src/build/matches/mod.rs
+++ b/compiler/rustc_mir_build/src/build/matches/mod.rs
@@ -1676,10 +1676,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&'b mut [&'c mut Candidate<'pat, 'tcx>],
FxIndexMap<TestBranch<'tcx>, 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();
@@ -1691,7 +1690,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;
}
@@ -1832,31 +1831,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<TestBranch<'tcx>, 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 = <N>
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<TestBranch<'tcx>> {
- 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::<i32>::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::<i32>::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<u32>));
-- 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<u32>));
+ StorageLive(_12);
@@ -48,12 +48,12 @@
bb2: {
- _6 = discriminant((_3.1: std::option::Option<u32>));
-- switchInt(move _6) -> [0: bb5, otherwise: bb1];
+- switchInt(move _6) -> [1: bb4, otherwise: bb1];
- }
-
- bb3: {
- _7 = discriminant((_3.1: std::option::Option<u32>));
-- 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<u32>));
- 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<u32>));
- 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<u32>));
+ switchInt(move _7) -> [0: bb4, 1: bb7, otherwise: bb1];
}
bb4: {
- _7 = discriminant((_3.1: std::option::Option<u32>));
- switchInt(move _7) -> [0: bb6, 1: bb5, otherwise: bb1];
+ _0 = const 3_u32;
+ goto -> bb8;
}
bb5: {
From 3f01d6563842252fe6c1c43c878b4eb4f3a27ce8 Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Fri, 1 Mar 2024 01:59:04 +0100
Subject: [PATCH 8/9] 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 aeadd6c770318..3dfc0611eb672 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, *};
@@ -1138,19 +1137,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<VariantIdx>,
},
/// 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<Const<'tcx>, u128>,
- },
+ SwitchInt,
/// Test what value a `bool` has.
If,
@@ -1195,6 +1185,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)]
@@ -1606,30 +1602,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)
}
@@ -1686,7 +1661,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<Const<'tcx>, 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<VariantIdx>,
- ) -> 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<TestBranch<'tcx>, Vec<&mut Candidate<'_, 'tcx>>>,
) -> Option<TestBranch<'tcx>> {
// 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<Const<'tcx>, u128>,
- ) -> Option<bool> {
- 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 6433f904c25c2949e6d9ced04fc103594df12ba4 Mon Sep 17 00:00:00 2001
From: Nadrieril <nadrieril+git@gmail.com>
Date: Sat, 2 Mar 2024 02:49:33 +0100
Subject: [PATCH 9/9] 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 3dfc0611eb672..f572ba1438074 100644
--- a/compiler/rustc_mir_build/src/build/matches/mod.rs
+++ b/compiler/rustc_mir_build/src/build/matches/mod.rs
@@ -1113,6 +1113,12 @@ enum TestCase<'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!(),
+ }
+}