coverage: Simplify the loop that combines blocks into BCBs

The old loop had two separate places where it would flush the acumulated list
of straight-line blocks into a new BCB. One occurred at the start of the loop
body when the current block couldn't be chained into, and the other occurred at
the end of the loop body when the current block couldn't be chained from.

The latter check can be hoisted to the start of the loop body by making it
examine the previous block (which has added itself to the list) instead of the
current block. With that done, we can combine the two separate flushes into one
flush with two possible trigger conditions.

Author: Zalathar

compiler/rustc_mir_transform/src/coverage/graph.rs

@@ -91,74 +91,41 @@ impl CoverageGraph {
         // FIXME(#78544): MIR InstrumentCoverage: Improve coverage of `#[should_panic]` tests and
         // `catch_unwind()` handlers.
 
+        // Accumulates a chain of blocks that will be combined into one BCB.
         let mut basic_blocks = Vec::new();
+
         let filtered_successors = |bb| bcb_filtered_successors(mir_body[bb].terminator());
         for bb in short_circuit_preorder(mir_body, filtered_successors)
             .filter(|&bb| mir_body[bb].terminator().kind != TerminatorKind::Unreachable)
         {
-            if let Some(last) = basic_blocks.last() {
-                let predecessors = &mir_body.basic_blocks.predecessors()[bb];
-                if predecessors.len() > 1 || !predecessors.contains(last) {
-                    // The `bb` has more than one _incoming_ edge, and should start its own
-                    // `BasicCoverageBlockData`. (Note, the `basic_blocks` vector does not yet
-                    // include `bb`; it contains a sequence of one or more sequential basic_blocks
-                    // with no intermediate branches in or out. Save these as a new
-                    // `BasicCoverageBlockData` before starting the new one.)
-                    Self::add_basic_coverage_block(
-                        &mut bcbs,
-                        &mut bb_to_bcb,
-                        basic_blocks.split_off(0),
-                    );
-                    debug!(
-                        "  because {}",
-                        if predecessors.len() > 1 {
-                            "predecessors.len() > 1".to_owned()
-                        } else {
-                            format!("bb {} is not in predecessors: {:?}", bb.index(), predecessors)
-                        }
-                    );
-                }
+            // If the previous block can't be chained into `bb`, flush the accumulated
+            // blocks into a new BCB, then start building the next chain.
+            if let Some(&prev) = basic_blocks.last()
+                && (!filtered_successors(prev).is_chainable() || {
+                    // If `bb` has multiple predecessor blocks, or `prev` isn't
+                    // one of its predecessors, we can't chain and must flush.
+                    let predecessors = &mir_body.basic_blocks.predecessors()[bb];
+                    predecessors.len() > 1 || !predecessors.contains(&prev)
+                })
+            {
+                debug!(
+                    terminator_kind = ?mir_body[prev].terminator().kind,
+                    predecessors = ?&mir_body.basic_blocks.predecessors()[bb],
+                    "can't chain from {prev:?} to {bb:?}"
+                );
+                Self::add_basic_coverage_block(
+                    &mut bcbs,
+                    &mut bb_to_bcb,
+                    basic_blocks.split_off(0),
+                );
             }
-            basic_blocks.push(bb);
-
-            let term = mir_body[bb].terminator();
-
-            match bcb_filtered_successors(term) {
-                CoverageSuccessors::NotChainable(_) => {
-                    // The `bb` has more than one _outgoing_ edge, or exits the function. Save the
-                    // current sequence of `basic_blocks` gathered to this point, as a new
-                    // `BasicCoverageBlockData`.
-                    Self::add_basic_coverage_block(
-                        &mut bcbs,
-                        &mut bb_to_bcb,
-                        basic_blocks.split_off(0),
-                    );
-                    debug!("  because term.kind = {:?}", term.kind);
-                    // Note that this condition is based on `TerminatorKind`, even though it
-                    // theoretically boils down to `successors().len() != 1`; that is, either zero
-                    // (e.g., `Return`, `Terminate`) or multiple successors (e.g., `SwitchInt`), but
-                    // since the BCB CFG ignores things like unwind branches (which exist in the
-                    // `Terminator`s `successors()` list) checking the number of successors won't
-                    // work.
-                }
 
-                // The following `TerminatorKind`s are either not expected outside an unwind branch,
-                // or they should not (under normal circumstances) branch. Coverage graphs are
-                // simplified by assuring coverage results are accurate for program executions that
-                // don't panic.
-                //
-                // Programs that panic and unwind may record slightly inaccurate coverage results
-                // for a coverage region containing the `Terminator` that began the panic. This
-                // is as intended. (See Issue #78544 for a possible future option to support
-                // coverage in test programs that panic.)
-                CoverageSuccessors::Chainable(_) => {}
-            }
+            basic_blocks.push(bb);
         }
 
         if !basic_blocks.is_empty() {
-            // process any remaining basic_blocks into a final `BasicCoverageBlockData`
+            debug!("flushing accumulated blocks into one last BCB");
             Self::add_basic_coverage_block(&mut bcbs, &mut bb_to_bcb, basic_blocks.split_off(0));
-            debug!("  because the end of the MIR CFG was reached while traversing");
         }
 
         (bcbs, bb_to_bcb)
@@ -350,6 +317,15 @@ enum CoverageSuccessors<'a> {
     NotChainable(&'a [BasicBlock]),
 }
 
+impl CoverageSuccessors<'_> {
+    fn is_chainable(&self) -> bool {
+        match self {
+            Self::Chainable(_) => true,
+            Self::NotChainable(_) => false,
+        }
+    }
+}
+
 impl IntoIterator for CoverageSuccessors<'_> {
     type Item = BasicBlock;
     type IntoIter = impl DoubleEndedIterator<Item = Self::Item>;