Rollup merge of #120978 - Nadrieril:sane-blocks, r=matthewjasper

match lowering: simplify block creation

Match lowering was doing complicated things with block creation. As far as I can tell it was trying to avoid creating unneeded blocks, but of the three places that start out with `otherwise = &mut None`, two of them called `otherwise.unwrap_or_else(|| self.cfg.start_new_block())` anyway. As far as I can tell the only place where this PR makes a difference is in `lower_match_tree`, which did indeed sometimes avoid creating the unreachable final block + FakeRead. Unless this is important I propose we do the naive thing instead.

I have not checked all the graph isomorphisms by hand, but at a glance the test diff looks sensible.

r? `@matthewjasper`

Author: matthiaskrgr

compiler/rustc_mir_build/src/build/matches/mod.rs

@@ -319,54 +319,52 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // them.
         let mut fake_borrows = match_has_guard.then(FxIndexSet::default);
 
-        let mut otherwise = None;
+        let otherwise_block = self.cfg.start_new_block();
 
         // This will generate code to test scrutinee_place and
         // branch to the appropriate arm block
         self.match_candidates(
             match_start_span,
             scrutinee_span,
             block,
-            &mut otherwise,
+            otherwise_block,
             candidates,
             &mut fake_borrows,
         );
 
-        if let Some(otherwise_block) = otherwise {
-            // See the doc comment on `match_candidates` for why we may have an
-            // otherwise block. Match checking will ensure this is actually
-            // unreachable.
-            let source_info = self.source_info(scrutinee_span);
-
-            // Matching on a `scrutinee_place` with an uninhabited type doesn't
-            // generate any memory reads by itself, and so if the place "expression"
-            // contains unsafe operations like raw pointer dereferences or union
-            // field projections, we wouldn't know to require an `unsafe` block
-            // around a `match` equivalent to `std::intrinsics::unreachable()`.
-            // See issue #47412 for this hole being discovered in the wild.
-            //
-            // HACK(eddyb) Work around the above issue by adding a dummy inspection
-            // of `scrutinee_place`, specifically by applying `ReadForMatch`.
-            //
-            // NOTE: ReadForMatch also checks that the scrutinee is initialized.
-            // This is currently needed to not allow matching on an uninitialized,
-            // uninhabited value. If we get never patterns, those will check that
-            // the place is initialized, and so this read would only be used to
-            // check safety.
-            let cause_matched_place = FakeReadCause::ForMatchedPlace(None);
-
-            if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) {
-                self.cfg.push_fake_read(
-                    otherwise_block,
-                    source_info,
-                    cause_matched_place,
-                    scrutinee_place,
-                );
-            }
+        // See the doc comment on `match_candidates` for why we may have an
+        // otherwise block. Match checking will ensure this is actually
+        // unreachable.
+        let source_info = self.source_info(scrutinee_span);
+
+        // Matching on a `scrutinee_place` with an uninhabited type doesn't
+        // generate any memory reads by itself, and so if the place "expression"
+        // contains unsafe operations like raw pointer dereferences or union
+        // field projections, we wouldn't know to require an `unsafe` block
+        // around a `match` equivalent to `std::intrinsics::unreachable()`.
+        // See issue #47412 for this hole being discovered in the wild.
+        //
+        // HACK(eddyb) Work around the above issue by adding a dummy inspection
+        // of `scrutinee_place`, specifically by applying `ReadForMatch`.
+        //
+        // NOTE: ReadForMatch also checks that the scrutinee is initialized.
+        // This is currently needed to not allow matching on an uninitialized,
+        // uninhabited value. If we get never patterns, those will check that
+        // the place is initialized, and so this read would only be used to
+        // check safety.
+        let cause_matched_place = FakeReadCause::ForMatchedPlace(None);
 
-            self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable);
+        if let Some(scrutinee_place) = scrutinee_place_builder.try_to_place(self) {
+            self.cfg.push_fake_read(
+                otherwise_block,
+                source_info,
+                cause_matched_place,
+                scrutinee_place,
+            );
         }
 
+        self.cfg.terminate(otherwise_block, source_info, TerminatorKind::Unreachable);
+
         // Link each leaf candidate to the `pre_binding_block` of the next one.
         let mut previous_candidate: Option<&mut Candidate<'_, '_>> = None;
 
@@ -1163,7 +1161,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         span: Span,
         scrutinee_span: Span,
         start_block: BasicBlock,
-        otherwise_block: &mut Option<BasicBlock>,
+        otherwise_block: BasicBlock,
         candidates: &mut [&mut Candidate<'pat, 'tcx>],
         fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
     ) {
@@ -1210,7 +1208,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         span: Span,
         scrutinee_span: Span,
         start_block: BasicBlock,
-        otherwise_block: &mut Option<BasicBlock>,
+        otherwise_block: BasicBlock,
         candidates: &mut [&mut Candidate<'_, 'tcx>],
         fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
     ) {
@@ -1243,11 +1241,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         // never reach this point.
         if unmatched_candidates.is_empty() {
             let source_info = self.source_info(span);
-            if let Some(otherwise) = *otherwise_block {
-                self.cfg.goto(block, source_info, otherwise);
-            } else {
-                *otherwise_block = Some(block);
-            }
+            self.cfg.goto(block, source_info, otherwise_block);
             return;
         }
 
@@ -1428,7 +1422,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         scrutinee_span: Span,
         candidates: &mut [&mut Candidate<'_, 'tcx>],
         block: BasicBlock,
-        otherwise_block: &mut Option<BasicBlock>,
+        otherwise_block: BasicBlock,
         fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
     ) {
         let (first_candidate, remaining_candidates) = candidates.split_first_mut().unwrap();
@@ -1453,7 +1447,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         let match_pairs = mem::take(&mut first_candidate.match_pairs);
         first_candidate.pre_binding_block = Some(block);
 
-        let mut otherwise = None;
+        let remainder_start = self.cfg.start_new_block();
         for match_pair in match_pairs {
             let PatKind::Or { ref pats } = &match_pair.pattern.kind else {
                 bug!("Or-patterns should have been sorted to the end");
@@ -1463,7 +1457,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             first_candidate.visit_leaves(|leaf_candidate| {
                 self.test_or_pattern(
                     leaf_candidate,
-                    &mut otherwise,
+                    remainder_start,
                     pats,
                     or_span,
                     &match_pair.place,
@@ -1472,8 +1466,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             });
         }
 
-        let remainder_start = otherwise.unwrap_or_else(|| self.cfg.start_new_block());
-
         self.match_candidates(
             span,
             scrutinee_span,
@@ -1491,7 +1483,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     fn test_or_pattern<'pat>(
         &mut self,
         candidate: &mut Candidate<'pat, 'tcx>,
-        otherwise: &mut Option<BasicBlock>,
+        otherwise: BasicBlock,
         pats: &'pat [Box<Pat<'tcx>>],
         or_span: Span,
         place: &PlaceBuilder<'tcx>,
@@ -1503,8 +1495,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             .map(|pat| Candidate::new(place.clone(), pat, candidate.has_guard, self))
             .collect();
         let mut or_candidate_refs: Vec<_> = or_candidates.iter_mut().collect();
-        let otherwise = if candidate.otherwise_block.is_some() {
-            &mut candidate.otherwise_block
+        let otherwise = if let Some(otherwise_block) = candidate.otherwise_block {
+            otherwise_block
         } else {
             otherwise
         };
@@ -1680,8 +1672,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         span: Span,
         scrutinee_span: Span,
         mut candidates: &'b mut [&'c mut Candidate<'pat, 'tcx>],
-        block: BasicBlock,
-        otherwise_block: &mut Option<BasicBlock>,
+        start_block: BasicBlock,
+        otherwise_block: BasicBlock,
         fake_borrows: &mut Option<FxIndexSet<Place<'tcx>>>,
     ) {
         // extract the match-pair from the highest priority candidate
@@ -1749,12 +1741,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
         debug!("untested_candidates: {}", candidates.len());
 
         // The block that we should branch to if none of the
-        // `target_candidates` match. This is either the block where we
-        // start matching the untested candidates if there are any,
-        // otherwise it's the `otherwise_block`.
-        let remainder_start = &mut None;
-        let remainder_start =
-            if candidates.is_empty() { &mut *otherwise_block } else { remainder_start };
+        // `target_candidates` match.
+        let remainder_start = if !candidates.is_empty() {
+            let remainder_start = self.cfg.start_new_block();
+            self.match_candidates(
+                span,
+                scrutinee_span,
+                remainder_start,
+                otherwise_block,
+                candidates,
+                fake_borrows,
+            );
+            remainder_start
+        } else {
+            otherwise_block
+        };
 
         // For each outcome of test, process the candidates that still
         // apply. Collect a list of blocks where control flow will
@@ -1775,24 +1776,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                     );
                     candidate_start
                 } else {
-                    *remainder_start.get_or_insert_with(|| self.cfg.start_new_block())
+                    remainder_start
                 }
             })
             .collect();
 
-        if !candidates.is_empty() {
-            let remainder_start = remainder_start.unwrap_or_else(|| self.cfg.start_new_block());
-            self.match_candidates(
-                span,
-                scrutinee_span,
-                remainder_start,
-                otherwise_block,
-                candidates,
-                fake_borrows,
-            );
-        }
-
-        self.perform_test(span, scrutinee_span, block, &match_place, &test, target_blocks);
+        // Perform the test, branching to one of N blocks.
+        self.perform_test(span, scrutinee_span, start_block, &match_place, &test, target_blocks);
     }
 
     /// Determine the fake borrows that are needed from a set of places that

tests/mir-opt/building/async_await.b-{closure#0}.coroutine_resume.0.mir

@@ -110,7 +110,7 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,
 
     bb0: {
         _39 = discriminant((*(_1.0: &mut {async fn body@$DIR/async_await.rs:15:18: 18:2})));
-        switchInt(move _39) -> [0: bb1, 1: bb29, 3: bb27, 4: bb28, otherwise: bb9];
+        switchInt(move _39) -> [0: bb1, 1: bb29, 3: bb27, 4: bb28, otherwise: bb8];
     }
 
     bb1: {
@@ -165,10 +165,14 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,
         StorageDead(_10);
         PlaceMention(_9);
         _16 = discriminant(_9);
-        switchInt(move _16) -> [0: bb10, 1: bb8, otherwise: bb9];
+        switchInt(move _16) -> [0: bb10, 1: bb9, otherwise: bb8];
     }
 
     bb8: {
+        unreachable;
+    }
+
+    bb9: {
         _8 = const ();
         StorageDead(_14);
         StorageDead(_12);
@@ -186,10 +190,6 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,
         return;
     }
 
-    bb9: {
-        unreachable;
-    }
-
     bb10: {
         StorageLive(_17);
         _17 = ((_9 as Ready).0: ());
@@ -267,7 +267,7 @@ fn b::{closure#0}(_1: Pin<&mut {async fn body@$DIR/async_await.rs:15:18: 18:2}>,
         StorageDead(_26);
         PlaceMention(_25);
         _32 = discriminant(_25);
-        switchInt(move _32) -> [0: bb21, 1: bb20, otherwise: bb9];
+        switchInt(move _32) -> [0: bb21, 1: bb20, otherwise: bb8];
     }
 
     bb20: {

tests/mir-opt/building/issue_101867.main.built.after.mir

@@ -27,13 +27,13 @@ fn main() -> () {
         StorageLive(_5);
         PlaceMention(_1);
         _6 = discriminant(_1);
-        switchInt(move _6) -> [1: bb4, otherwise: bb3];
+        switchInt(move _6) -> [1: bb5, otherwise: bb4];
     }
 
     bb1: {
         StorageLive(_3);
         StorageLive(_4);
-        _4 = begin_panic::<&str>(const "explicit panic") -> bb7;
+        _4 = begin_panic::<&str>(const "explicit panic") -> bb8;
     }
 
     bb2: {
@@ -43,27 +43,32 @@ fn main() -> () {
     }
 
     bb3: {
-        goto -> bb6;
+        FakeRead(ForMatchedPlace(None), _1);
+        unreachable;
     }
 
     bb4: {
-        falseEdge -> [real: bb5, imaginary: bb3];
+        goto -> bb7;
     }
 
     bb5: {
+        falseEdge -> [real: bb6, imaginary: bb4];
+    }
+
+    bb6: {
         _5 = ((_1 as Some).0: u8);
         _0 = const ();
         StorageDead(_5);
         StorageDead(_1);
         return;
     }
 
-    bb6: {
+    bb7: {
         StorageDead(_5);
         goto -> bb1;
     }
 
-    bb7 (cleanup): {
+    bb8 (cleanup): {
         resume;
     }
 }