Skip to content

Commit

Permalink
Auto merge of #121172 - Nadrieril:simplify-empty-selection, r=<try>
Browse files Browse the repository at this point in the history 
match lowering: simplify empty candidate selection

In match lowering, `match_simplified_candidates` is tasked with removing candidates that are fully matched and linking them up properly. The code that does that was needlessly complicated; this PR simplifies it.

The overall change isn't big but I split it up into tiny commits to convince myself that I was correctly preserving behavior. The test changes are all due to the first commit. Let me know if you'd prefer me to split up the PR to make reviewing easier.

r? `@matthewjasper`
  • Loading branch information
@bors
bors committed Feb 16, 2024
2 parents 0f806a9 + 4a9cff7 commit 1d1bfb7
Show file tree
Hide file tree
Showing 16 changed files with 443 additions and 365 deletions.
162 changes: 62 additions & 100 deletions compiler/rustc_mir_build/src/build/matches/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1207,53 +1207,43 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&mut self,
span: Span,
scrutinee_span: Span,
start_block: BasicBlock,
mut start_block: BasicBlock,
otherwise_block: BasicBlock,
candidates: &mut [&mut Candidate<'_, 'tcx>],
fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
) {
// The candidates are sorted by priority. Check to see whether the
// higher priority candidates (and hence at the front of the slice)
// have satisfied all their match pairs.
let fully_matched = candidates.iter().take_while(|c| c.match_pairs.is_empty()).count();
debug!("match_candidates: {:?} candidates fully matched", fully_matched);
let (matched_candidates, unmatched_candidates) = candidates.split_at_mut(fully_matched);

let block = if !matched_candidates.is_empty() {
let otherwise_block =
self.select_matched_candidates(matched_candidates, start_block, fake_borrows);

if let Some(last_otherwise_block) = otherwise_block {
last_otherwise_block
} else {
// Any remaining candidates are unreachable.
if unmatched_candidates.is_empty() {
return;
}
self.cfg.start_new_block()
match candidates {
[] => {
// If there are no candidates that still need testing, we're done. Since all matches are
// exhaustive, execution should never reach this point.
let source_info = self.source_info(span);
self.cfg.goto(start_block, source_info, otherwise_block);
}
[first, remaining @ ..] if first.match_pairs.is_empty() => {
// The first candidate has satisfied all its match pairs; we link it up and continue
// with the remaining candidates.
start_block = self.select_matched_candidate(first, start_block, fake_borrows);
self.match_simplified_candidates(
span,
scrutinee_span,
start_block,
otherwise_block,
remaining,
fake_borrows,
)
}
candidates => {
// The first candidate has some unsatisfied match pairs; we proceed to do more tests.
self.test_candidates_with_or(
span,
scrutinee_span,
candidates,
start_block,
otherwise_block,
fake_borrows,
);
}
} else {
start_block
};

// If there are no candidates that still need testing, we're
// done. Since all matches are exhaustive, execution should
// never reach this point.
if unmatched_candidates.is_empty() {
let source_info = self.source_info(span);
self.cfg.goto(block, source_info, otherwise_block);
return;
}

// Test for the remaining candidates.
self.test_candidates_with_or(
span,
scrutinee_span,
unmatched_candidates,
block,
otherwise_block,
fake_borrows,
);
}

/// Link up matched candidates.
Expand All @@ -1275,47 +1265,43 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
/// * the [otherwise block] of the third pattern to a block with an
/// [`Unreachable` terminator](TerminatorKind::Unreachable).
///
/// In addition, we add fake edges from the otherwise blocks to the
/// In addition, we later add fake edges from the otherwise blocks to the
/// pre-binding block of the next candidate in the original set of
/// candidates.
///
/// [pre-binding block]: Candidate::pre_binding_block
/// [otherwise block]: Candidate::otherwise_block
fn select_matched_candidates(
fn select_matched_candidate(
&mut self,
matched_candidates: &mut [&mut Candidate<'_, 'tcx>],
candidate: &mut Candidate<'_, 'tcx>,
start_block: BasicBlock,
fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
) -> Option<BasicBlock> {
debug_assert!(
!matched_candidates.is_empty(),
"select_matched_candidates called with no candidates",
);
) -> BasicBlock {
assert!(candidate.otherwise_block.is_none());
assert!(candidate.pre_binding_block.is_none());
debug_assert!(
matched_candidates.iter().all(|c| c.subcandidates.is_empty()),
candidate.subcandidates.is_empty(),
"subcandidates should be empty in select_matched_candidates",
);

// Insert a borrows of prefixes of places that are bound and are
// behind a dereference projection.
//
// These borrows are taken to avoid situations like the following:
//
// match x[10] {
// _ if { x = &[0]; false } => (),
// y => (), // Out of bounds array access!
// }
//
// match *x {
// // y is bound by reference in the guard and then by copy in the
// // arm, so y is 2 in the arm!
// y if { y == 1 && (x = &2) == () } => y,
// _ => 3,
// }
if let Some(fake_borrows) = fake_borrows {
for Binding { source, .. } in
matched_candidates.iter().flat_map(|candidate| &candidate.bindings)
{
// Insert a borrows of prefixes of places that are bound and are
// behind a dereference projection.
//
// These borrows are taken to avoid situations like the following:
//
// match x[10] {
// _ if { x = &[0]; false } => (),
// y => (), // Out of bounds array access!
// }
//
// match *x {
// // y is bound by reference in the guard and then by copy in the
// // arm, so y is 2 in the arm!
// y if { y == 1 && (x = &2) == () } => y,
// _ => 3,
// }
for Binding { source, .. } in &candidate.bindings {
if let Some(i) =
source.projection.iter().rposition(|elem| elem == ProjectionElem::Deref)
{
Expand All @@ -1329,38 +1315,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}

let fully_matched_with_guard = matched_candidates
.iter()
.position(|c| !c.has_guard)
.unwrap_or(matched_candidates.len() - 1);

let (reachable_candidates, unreachable_candidates) =
matched_candidates.split_at_mut(fully_matched_with_guard + 1);

let mut next_prebinding = start_block;

for candidate in reachable_candidates.iter_mut() {
assert!(candidate.otherwise_block.is_none());
assert!(candidate.pre_binding_block.is_none());
candidate.pre_binding_block = Some(next_prebinding);
if candidate.has_guard {
// Create the otherwise block for this candidate, which is the
// pre-binding block for the next candidate.
next_prebinding = self.cfg.start_new_block();
candidate.otherwise_block = Some(next_prebinding);
}
}

debug!(
"match_candidates: add pre_binding_blocks for unreachable {:?}",
unreachable_candidates,
);
for candidate in unreachable_candidates {
assert!(candidate.pre_binding_block.is_none());
candidate.pre_binding_block = Some(self.cfg.start_new_block());
candidate.pre_binding_block = Some(start_block);
let otherwise_block = self.cfg.start_new_block();
if candidate.has_guard {
// Create the otherwise block for this candidate, which is the
// pre-binding block for the next candidate.
candidate.otherwise_block = Some(otherwise_block);
}

reachable_candidates.last_mut().unwrap().otherwise_block
otherwise_block
}

/// Tests a candidate where there are only or-patterns left to test, or
Expand Down
20 changes: 14 additions & 6 deletions tests/mir-opt/building/issue_101867.main.built.after.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,13 @@ fn main() -> () {
StorageLive(_5);
PlaceMention(_1);
_6 = discriminant(_1);
switchInt(move _6) -> [1: bb5, otherwise: bb4];
switchInt(move _6) -> [1: bb6, otherwise: bb4];
}

bb1: {
StorageLive(_3);
StorageLive(_4);
_4 = begin_panic::<&str>(const "explicit panic") -> bb8;
_4 = begin_panic::<&str>(const "explicit panic") -> bb10;
}

bb2: {
Expand All @@ -48,27 +48,35 @@ fn main() -> () {
}

bb4: {
goto -> bb7;
goto -> bb9;
}

bb5: {
falseEdge -> [real: bb6, imaginary: bb4];
goto -> bb3;
}

bb6: {
falseEdge -> [real: bb8, imaginary: bb4];
}

bb7: {
goto -> bb4;
}

bb8: {
_5 = ((_1 as Some).0: u8);
_0 = const ();
StorageDead(_5);
StorageDead(_1);
return;
}

bb7: {
bb9: {
StorageDead(_5);
goto -> bb1;
}

bb8 (cleanup): {
bb10 (cleanup): {
resume;
}
}
34 changes: 21 additions & 13 deletions tests/mir-opt/building/issue_49232.main.built.after.mir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,15 @@ fn main() -> () {
}

bb1: {
falseUnwind -> [real: bb2, unwind: bb12];
falseUnwind -> [real: bb2, unwind: bb14];
}

bb2: {
StorageLive(_2);
StorageLive(_3);
_3 = const true;
PlaceMention(_3);
switchInt(_3) -> [0: bb4, otherwise: bb5];
switchInt(_3) -> [0: bb4, otherwise: bb6];
}

bb3: {
Expand All @@ -34,51 +34,59 @@ fn main() -> () {
}

bb4: {
falseEdge -> [real: bb6, imaginary: bb5];
falseEdge -> [real: bb8, imaginary: bb6];
}

bb5: {
_0 = const ();
goto -> bb11;
goto -> bb3;
}

bb6: {
_2 = const 4_i32;
goto -> bb9;
_0 = const ();
goto -> bb13;
}

bb7: {
unreachable;
goto -> bb3;
}

bb8: {
goto -> bb9;
_2 = const 4_i32;
goto -> bb11;
}

bb9: {
unreachable;
}

bb10: {
goto -> bb11;
}

bb11: {
FakeRead(ForLet(None), _2);
StorageDead(_3);
StorageLive(_5);
StorageLive(_6);
_6 = &_2;
_5 = std::mem::drop::<&i32>(move _6) -> [return: bb10, unwind: bb12];
_5 = std::mem::drop::<&i32>(move _6) -> [return: bb12, unwind: bb14];
}

bb10: {
bb12: {
StorageDead(_6);
StorageDead(_5);
_1 = const ();
StorageDead(_2);
goto -> bb1;
}

bb11: {
bb13: {
StorageDead(_3);
StorageDead(_2);
return;
}

bb12 (cleanup): {
bb14 (cleanup): {
resume;
}
}
Loading

0 comments on commit 1d1bfb7

Please sign in to comment.