From 3b9d7037c9aa22b06620499c548f5c53b104de33 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 1 Nov 2023 11:40:12 +1100 Subject: [PATCH] coverage: Eliminate `BcbBranch` `BcbBranch` represented an out-edge of a coverage graph node, but would silently refer to a node instead in cases where that node only had one in-edge. Instead we now refer to a graph edge as a `(from_bcb, to_bcb)` pair, or sometimes as just one of those nodes when the other node is implied by the surrounding context. The case of sole in-edges is handled by special code added directly to `get_or_make_edge_counter_operand`. --- .../src/coverage/counters.rs | 134 +++++++++--------- .../rustc_mir_transform/src/coverage/graph.rs | 39 ----- 2 files changed, 69 insertions(+), 104 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index b5fba797dc4..604589e5b96 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -1,13 +1,11 @@ -use super::graph; - -use graph::{BasicCoverageBlock, BcbBranch, CoverageGraph, TraverseCoverageGraphWithLoops}; - use rustc_data_structures::fx::FxHashMap; use rustc_data_structures::graph::WithNumNodes; use rustc_index::bit_set::BitSet; use rustc_index::IndexVec; use rustc_middle::mir::coverage::*; +use super::graph::{BasicCoverageBlock, CoverageGraph, TraverseCoverageGraphWithLoops}; + use std::fmt::{self, Debug}; /// The coverage counter or counter expression associated with a particular @@ -259,49 +257,46 @@ fn make_node_and_branch_counters( // We might also use its term later to compute one of the branch counters. let from_bcb_operand = self.get_or_make_counter_operand(from_bcb); - let branches = self.bcb_branches(from_bcb); + let branch_target_bcbs = self.basic_coverage_blocks.successors[from_bcb].as_slice(); // If this node doesn't have multiple out-edges, or all of its out-edges // already have counters, then we don't need to create edge counters. - let needs_branch_counters = - branches.len() > 1 && branches.iter().any(|branch| self.branch_has_no_counter(branch)); + let needs_branch_counters = branch_target_bcbs.len() > 1 + && branch_target_bcbs + .iter() + .any(|&to_bcb| self.branch_has_no_counter(from_bcb, to_bcb)); if !needs_branch_counters { return; } debug!( "{from_bcb:?} has some branch(es) without counters:\n {}", - branches + branch_target_bcbs .iter() - .map(|branch| { format!("{:?}: {:?}", branch, self.branch_counter(branch)) }) + .map(|&to_bcb| { + format!("{from_bcb:?}->{to_bcb:?}: {:?}", self.branch_counter(from_bcb, to_bcb)) + }) .collect::>() .join("\n "), ); - // Use the `traversal` state to decide if a subset of the branches exit a loop, making it - // likely that branch is executed less than branches that do not exit the same loop. In this - // case, any branch that does not exit the loop (and has not already been assigned a - // counter) should be counted by expression, if possible. (If a preferred expression branch - // is not selected based on the loop context, select any branch without an existing - // counter.) - let expression_branch = self.choose_preferred_expression_branch(traversal, &branches); + // Of the branch edges that don't have counters yet, one can be given an expression + // (computed from the other edges) instead of a dedicated counter. + let expression_to_bcb = self.choose_preferred_expression_branch(traversal, from_bcb); // For each branch arm other than the one that was chosen to get an expression, // ensure that it has a counter (existing counter/expression or a new counter), // and accumulate the corresponding terms into a single sum term. let sum_of_all_other_branches: BcbCounter = { - let _span = debug_span!("sum_of_all_other_branches", ?expression_branch).entered(); - branches - .into_iter() + let _span = debug_span!("sum_of_all_other_branches", ?expression_to_bcb).entered(); + branch_target_bcbs + .iter() + .copied() // Skip the chosen branch, since we'll calculate it from the other branches. - .filter(|branch| branch != &expression_branch) - .fold(None, |accum, branch| { - let _span = debug_span!("branch", ?accum, ?branch).entered(); - let branch_counter = if branch.is_only_path_to_target() { - self.get_or_make_counter_operand(branch.target_bcb) - } else { - self.get_or_make_edge_counter_operand(from_bcb, branch.target_bcb) - }; + .filter(|&to_bcb| to_bcb != expression_to_bcb) + .fold(None, |accum, to_bcb| { + let _span = debug_span!("to_bcb", ?accum, ?to_bcb).entered(); + let branch_counter = self.get_or_make_edge_counter_operand(from_bcb, to_bcb); Some(self.coverage_counters.make_sum_expression(accum, branch_counter)) }) .expect("there must be at least one other branch") @@ -311,22 +306,20 @@ fn make_node_and_branch_counters( // by taking the count of the node we're branching from, and subtracting the // sum of all the other branches. debug!( - "Making an expression for the selected expression_branch: {:?} \ - (expression_branch predecessors: {:?})", - expression_branch, - self.bcb_predecessors(expression_branch.target_bcb), + "Making an expression for the selected expression_branch: \ + {expression_to_bcb:?} (expression_branch predecessors: {:?})", + self.bcb_predecessors(expression_to_bcb), ); let expression = self.coverage_counters.make_expression( from_bcb_operand, Op::Subtract, sum_of_all_other_branches, ); - debug!("{:?} gets an expression: {:?}", expression_branch, expression); - let bcb = expression_branch.target_bcb; - if expression_branch.is_only_path_to_target() { - self.coverage_counters.set_bcb_counter(bcb, expression); + debug!("{expression_to_bcb:?} gets an expression: {expression:?}"); + if self.basic_coverage_blocks.bcb_has_multiple_in_edges(expression_to_bcb) { + self.coverage_counters.set_bcb_edge_counter(from_bcb, expression_to_bcb, expression); } else { - self.coverage_counters.set_bcb_edge_counter(from_bcb, bcb, expression); + self.coverage_counters.set_bcb_counter(expression_to_bcb, expression); } } @@ -383,10 +376,16 @@ fn get_or_make_edge_counter_operand( from_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock, ) -> BcbCounter { + // If the target BCB has only one in-edge (i.e. this one), then create + // a node counter instead, since it will have the same value. + if !self.basic_coverage_blocks.bcb_has_multiple_in_edges(to_bcb) { + assert_eq!([from_bcb].as_slice(), self.basic_coverage_blocks.predecessors[to_bcb]); + return self.get_or_make_counter_operand(to_bcb); + } + // If the source BCB has only one successor (assumed to be the given target), an edge // counter is unnecessary. Just get or make a counter for the source BCB. - let successors = self.bcb_successors(from_bcb).iter(); - if successors.len() == 1 { + if self.bcb_successors(from_bcb).len() == 1 { return self.get_or_make_counter_operand(from_bcb); } @@ -409,16 +408,19 @@ fn get_or_make_edge_counter_operand( fn choose_preferred_expression_branch( &self, traversal: &TraverseCoverageGraphWithLoops<'_>, - branches: &[BcbBranch], - ) -> BcbBranch { - let good_reloop_branch = self.find_good_reloop_branch(traversal, branches); - if let Some(reloop_branch) = good_reloop_branch { - assert!(self.branch_has_no_counter(&reloop_branch)); - debug!("Selecting reloop branch {reloop_branch:?} to get an expression"); - reloop_branch + from_bcb: BasicCoverageBlock, + ) -> BasicCoverageBlock { + let good_reloop_branch = self.find_good_reloop_branch(traversal, from_bcb); + if let Some(reloop_target) = good_reloop_branch { + assert!(self.branch_has_no_counter(from_bcb, reloop_target)); + debug!("Selecting reloop target {reloop_target:?} to get an expression"); + reloop_target } else { - let &branch_without_counter = - branches.iter().find(|&branch| self.branch_has_no_counter(branch)).expect( + let &branch_without_counter = self + .bcb_successors(from_bcb) + .iter() + .find(|&&to_bcb| self.branch_has_no_counter(from_bcb, to_bcb)) + .expect( "needs_branch_counters was `true` so there should be at least one \ branch", ); @@ -439,26 +441,28 @@ fn choose_preferred_expression_branch( fn find_good_reloop_branch( &self, traversal: &TraverseCoverageGraphWithLoops<'_>, - branches: &[BcbBranch], - ) -> Option { + from_bcb: BasicCoverageBlock, + ) -> Option { + let branch_target_bcbs = self.bcb_successors(from_bcb); + // Consider each loop on the current traversal context stack, top-down. for reloop_bcbs in traversal.reloop_bcbs_per_loop() { let mut all_branches_exit_this_loop = true; // Try to find a branch that doesn't exit this loop and doesn't // already have a counter. - for &branch in branches { + for &branch_target_bcb in branch_target_bcbs { // A branch is a reloop branch if it dominates any BCB that has // an edge back to the loop header. (Other branches are exits.) let is_reloop_branch = reloop_bcbs.iter().any(|&reloop_bcb| { - self.basic_coverage_blocks.dominates(branch.target_bcb, reloop_bcb) + self.basic_coverage_blocks.dominates(branch_target_bcb, reloop_bcb) }); if is_reloop_branch { all_branches_exit_this_loop = false; - if self.branch_has_no_counter(&branch) { + if self.branch_has_no_counter(from_bcb, branch_target_bcb) { // We found a good branch to be given an expression. - return Some(branch); + return Some(branch_target_bcb); } // Keep looking for another reloop branch without a counter. } else { @@ -491,20 +495,20 @@ fn bcb_successors(&self, bcb: BasicCoverageBlock) -> &[BasicCoverageBlock] { } #[inline] - fn bcb_branches(&self, from_bcb: BasicCoverageBlock) -> Vec { - self.bcb_successors(from_bcb) - .iter() - .map(|&to_bcb| BcbBranch::from_to(from_bcb, to_bcb, self.basic_coverage_blocks)) - .collect::>() + fn branch_has_no_counter( + &self, + from_bcb: BasicCoverageBlock, + to_bcb: BasicCoverageBlock, + ) -> bool { + self.branch_counter(from_bcb, to_bcb).is_none() } - fn branch_has_no_counter(&self, branch: &BcbBranch) -> bool { - self.branch_counter(branch).is_none() - } - - fn branch_counter(&self, branch: &BcbBranch) -> Option<&BcbCounter> { - let to_bcb = branch.target_bcb; - if let Some(from_bcb) = branch.edge_from_bcb { + fn branch_counter( + &self, + from_bcb: BasicCoverageBlock, + to_bcb: BasicCoverageBlock, + ) -> Option<&BcbCounter> { + if self.basic_coverage_blocks.bcb_has_multiple_in_edges(to_bcb) { self.coverage_counters.bcb_edge_counters.get(&(from_bcb, to_bcb)) } else { self.coverage_counters.bcb_counters[to_bcb].as_ref() diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index b3a69e9fa82..34ab2992e3b 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -333,45 +333,6 @@ pub fn last_bb(&self) -> BasicBlock { } } -/// Represents a successor from a branching BasicCoverageBlock (such as the arms of a `SwitchInt`) -/// as either the successor BCB itself, if it has only one incoming edge, or the successor _plus_ -/// the specific branching BCB, representing the edge between the two. The latter case -/// distinguishes this incoming edge from other incoming edges to the same `target_bcb`. -#[derive(Clone, Copy, PartialEq, Eq)] -pub(super) struct BcbBranch { - pub edge_from_bcb: Option, - pub target_bcb: BasicCoverageBlock, -} - -impl BcbBranch { - pub fn from_to( - from_bcb: BasicCoverageBlock, - to_bcb: BasicCoverageBlock, - basic_coverage_blocks: &CoverageGraph, - ) -> Self { - let edge_from_bcb = if basic_coverage_blocks.bcb_has_multiple_in_edges(from_bcb) { - Some(from_bcb) - } else { - None - }; - Self { edge_from_bcb, target_bcb: to_bcb } - } - - pub fn is_only_path_to_target(&self) -> bool { - self.edge_from_bcb.is_none() - } -} - -impl std::fmt::Debug for BcbBranch { - fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - if let Some(from_bcb) = self.edge_from_bcb { - write!(fmt, "{:?}->{:?}", from_bcb, self.target_bcb) - } else { - write!(fmt, "{:?}", self.target_bcb) - } - } -} - // Returns the subset of a block's successors that are relevant to the coverage // graph, i.e. those that do not represent unwinds or unreachable branches. // FIXME(#78544): MIR InstrumentCoverage: Improve coverage of `#[should_panic]` tests and