Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

match lowering: Split finalize_or_candidate into more coherent methods #127917

Merged
merged 5 commits into from
Jul 20, 2024
Merged

match lowering: Split finalize_or_candidate into more coherent methods #127917

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b7e4bc
Split out `remove_never_subcandidates`
Zalathar Jul 18, 2024
e091c35
Improve `merge_trivial_subcandidates`
Zalathar Jul 18, 2024
e60c5c1
Split out `test_remaining_match_pairs_after_or`
Zalathar Jul 18, 2024
886668c
Improve `test_remaining_match_pairs_after_or`
Zalathar Jul 18, 2024
239037e
Inline `finalize_or_candidate`
Zalathar Jul 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
220 changes: 129 additions & 91 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1598,6 +1598,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
for subcandidate in candidate.subcandidates.iter_mut() {
expanded_candidates.push(subcandidate);
}
// Note that the subcandidates have been added to `expanded_candidates`,
// but `candidate` itself has not. If the last candidate has more match pairs,
// they are handled separately by `test_remaining_match_pairs_after_or`.
} else {
// A candidate that doesn't start with an or-pattern has nothing to
// expand, so it is included in the post-expansion list as-is.
Expand All @@ -1613,19 +1616,28 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expanded_candidates.as_mut_slice(),
);

// Simplify subcandidates and process any leftover match pairs.
for candidate in candidates_to_expand {
// Postprocess subcandidates, and process any leftover match pairs.
// (Only the last candidate can possibly have more match pairs.)
debug_assert!({
let mut all_except_last = candidates_to_expand.iter().rev().skip(1);
all_except_last.all(|candidate| candidate.match_pairs.is_empty())
});
for candidate in candidates_to_expand.iter_mut() {
if !candidate.subcandidates.is_empty() {
self.finalize_or_candidate(span, scrutinee_span, candidate);
self.merge_trivial_subcandidates(candidate);
self.remove_never_subcandidates(candidate);
}
}
if let Some(last_candidate) = candidates_to_expand.last_mut() {
self.test_remaining_match_pairs_after_or(span, scrutinee_span, last_candidate);
Nadrieril marked this conversation as resolved.
Show resolved Hide resolved
}

remainder_start.and(remaining_candidates)
}

/// Given a match-pair that corresponds to an or-pattern, expand each subpattern into a new
/// subcandidate. Any candidate that has been expanded that way should be passed to
/// `finalize_or_candidate` after its subcandidates have been processed.
/// subcandidate. Any candidate that has been expanded this way should also be postprocessed
/// at the end of [`Self::expand_and_match_or_candidates`].
fn create_or_subcandidates<'pat>(
&mut self,
candidate: &mut Candidate<'pat, 'tcx>,
Expand All @@ -1642,7 +1654,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
candidate.subcandidates[0].false_edge_start_block = candidate.false_edge_start_block;
}

/// Simplify subcandidates and process any leftover match pairs. The candidate should have been
/// Try to merge all of the subcandidates of the given candidate into one. This avoids
/// exponentially large CFGs in cases like `(1 | 2, 3 | 4, ...)`. The candidate should have been
/// expanded with `create_or_subcandidates`.
///
/// Given a pattern `(P | Q, R | S)` we (in principle) generate a CFG like
Expand Down Expand Up @@ -1695,103 +1708,128 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
/// |
/// ...
/// ```
fn finalize_or_candidate(
&mut self,
span: Span,
scrutinee_span: Span,
candidate: &mut Candidate<'_, 'tcx>,
) {
if candidate.subcandidates.is_empty() {
///
/// Note that this takes place _after_ the subcandidates have participated
/// in match tree lowering.
fn merge_trivial_subcandidates(&mut self, candidate: &mut Candidate<'_, 'tcx>) {
assert!(!candidate.subcandidates.is_empty());
if candidate.has_guard {
// FIXME(or_patterns; matthewjasper) Don't give up if we have a guard.
return;
}

self.merge_trivial_subcandidates(candidate);
// FIXME(or_patterns; matthewjasper) Try to be more aggressive here.
let can_merge = candidate.subcandidates.iter().all(|subcandidate| {
subcandidate.subcandidates.is_empty() && subcandidate.extra_data.is_empty()
});
if !can_merge {
return;
}

if !candidate.match_pairs.is_empty() {
let or_span = candidate.or_span.unwrap_or(candidate.extra_data.span);
let source_info = self.source_info(or_span);
// 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 mut last_otherwise = None;
candidate.visit_leaves(|leaf_candidate| {
last_otherwise = leaf_candidate.otherwise_block;
});
let remaining_match_pairs = mem::take(&mut candidate.match_pairs);
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();
let otherwise =
self.match_candidates(span, scrutinee_span, or_start, &mut [leaf_candidate]);
// 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 `last_otherwise`. If there is a guard,
// `leaf_candidate.otherwise_block` can be reached by guard failure as well, so we
// can't skip `Q`.
let or_otherwise = if leaf_candidate.has_guard {
leaf_candidate.otherwise_block.unwrap()
} else {
last_otherwise.unwrap()
};
self.cfg.goto(otherwise, source_info, or_otherwise);
});
let mut last_otherwise = None;
let shared_pre_binding_block = self.cfg.start_new_block();
// This candidate is about to become a leaf, so unset `or_span`.
let or_span = candidate.or_span.take().unwrap();
let source_info = self.source_info(or_span);

if candidate.false_edge_start_block.is_none() {
candidate.false_edge_start_block = candidate.subcandidates[0].false_edge_start_block;
}

// Remove the (known-trivial) subcandidates from the candidate tree,
// so that they aren't visible after match tree lowering, and wire them
// all to join up at a single shared pre-binding block.
// (Note that the subcandidates have already had their part of the match
// tree lowered by this point, which is why we can add a goto to them.)
for subcandidate in mem::take(&mut candidate.subcandidates) {
let subcandidate_block = subcandidate.pre_binding_block.unwrap();
self.cfg.goto(subcandidate_block, source_info, shared_pre_binding_block);
last_otherwise = subcandidate.otherwise_block;
}
candidate.pre_binding_block = Some(shared_pre_binding_block);
assert!(last_otherwise.is_some());
candidate.otherwise_block = last_otherwise;
}

/// Try to merge all of the subcandidates of the given candidate into one. This avoids
/// exponentially large CFGs in cases like `(1 | 2, 3 | 4, ...)`. The candidate should have been
/// expanded with `create_or_subcandidates`.
fn merge_trivial_subcandidates(&mut self, candidate: &mut Candidate<'_, 'tcx>) {
if candidate.subcandidates.is_empty() || candidate.has_guard {
// FIXME(or_patterns; matthewjasper) Don't give up if we have a guard.
/// Never subcandidates may have a set of bindings inconsistent with their siblings,
/// which would break later code. So we filter them out. Note that we can't filter out
/// top-level candidates this way.
fn remove_never_subcandidates(&mut self, candidate: &mut Candidate<'_, 'tcx>) {
if candidate.subcandidates.is_empty() {
return;
}

// FIXME(or_patterns; matthewjasper) Try to be more aggressive here.
let can_merge = candidate.subcandidates.iter().all(|subcandidate| {
subcandidate.subcandidates.is_empty() && subcandidate.extra_data.is_empty()
});
if can_merge {
let mut last_otherwise = None;
let any_matches = self.cfg.start_new_block();
let or_span = candidate.or_span.take().unwrap();
let source_info = self.source_info(or_span);
if candidate.false_edge_start_block.is_none() {
candidate.false_edge_start_block =
candidate.subcandidates[0].false_edge_start_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);
last_otherwise = subcandidate.otherwise_block;
}
candidate.pre_binding_block = Some(any_matches);
assert!(last_otherwise.is_some());
candidate.otherwise_block = last_otherwise;
} else {
// Never subcandidates may have a set of bindings inconsistent with their siblings,
// which would break later code. So we filter them out. Note that we can't filter out
// top-level candidates this way.
candidate.subcandidates.retain_mut(|candidate| {
if candidate.extra_data.is_never {
candidate.visit_leaves(|subcandidate| {
let block = subcandidate.pre_binding_block.unwrap();
// That block is already unreachable but needs a terminator to make the MIR well-formed.
let source_info = self.source_info(subcandidate.extra_data.span);
self.cfg.terminate(block, source_info, TerminatorKind::Unreachable);
});
false
} else {
true
}
});
if candidate.subcandidates.is_empty() {
// If `candidate` has become a leaf candidate, ensure it has a `pre_binding_block`.
candidate.pre_binding_block = Some(self.cfg.start_new_block());
candidate.subcandidates.retain_mut(|candidate| {
if candidate.extra_data.is_never {
candidate.visit_leaves(|subcandidate| {
let block = subcandidate.pre_binding_block.unwrap();
// That block is already unreachable but needs a terminator to make the MIR well-formed.
let source_info = self.source_info(subcandidate.extra_data.span);
self.cfg.terminate(block, source_info, TerminatorKind::Unreachable);
});
false
} else {
true
}
});
if candidate.subcandidates.is_empty() {
// If `candidate` has become a leaf candidate, ensure it has a `pre_binding_block`.
candidate.pre_binding_block = Some(self.cfg.start_new_block());
}
}

/// If more match pairs remain, test them after each subcandidate.
/// We could have added them to the or-candidates during or-pattern expansion, but that
/// would make it impossible to detect simplifiable or-patterns. That would guarantee
/// exponentially large CFGs for cases like `(1 | 2, 3 | 4, ...)`.
fn test_remaining_match_pairs_after_or(
&mut self,
span: Span,
scrutinee_span: Span,
candidate: &mut Candidate<'_, 'tcx>,
) {
if candidate.match_pairs.is_empty() {
return;
}

let or_span = candidate.or_span.unwrap_or(candidate.extra_data.span);
let source_info = self.source_info(or_span);
let mut last_otherwise = None;
candidate.visit_leaves(|leaf_candidate| {
last_otherwise = leaf_candidate.otherwise_block;
});

let remaining_match_pairs = mem::take(&mut candidate.match_pairs);
// We're testing match pairs that remained after an `Or`, so the remaining
// pairs should all be `Or` too, due to the sorting invariant.
debug_assert!(
remaining_match_pairs
.iter()
.all(|match_pair| matches!(match_pair.test_case, TestCase::Or { .. }))
);

candidate.visit_leaves(|leaf_candidate| {
// At this point the leaf's own match pairs have all been lowered
// and removed, so `extend` and assignment are equivalent,
// but extending can also recycle any existing vector capacity.
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();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Side note: I wonder if this use of pre_binding_block should use Option::take instead, because it's no longer meaningful as a pre-binding block. Some quick testing indicates that doing so doesn't make tests fail.

(I won't make that change in this PR, but maybe I'll look into it later.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, maybe. And same for the otherwise_block that we use below, we may as well read it early.

let otherwise =
self.match_candidates(span, scrutinee_span, or_start, &mut [leaf_candidate]);
// 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 `last_otherwise`. If there is a guard,
// `leaf_candidate.otherwise_block` can be reached by guard failure as well, so we
// can't skip `Q`.
let or_otherwise = if leaf_candidate.has_guard {
leaf_candidate.otherwise_block.unwrap()
} else {
last_otherwise.unwrap()
};
self.cfg.goto(otherwise, source_info, or_otherwise);
});
}

/// Pick a test to run. Which test doesn't matter as long as it is guaranteed to fully match at
Expand Down
Loading