mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #130184 - Zalathar:counters, r=compiler-errors

Browse Source 
coverage: Clean up terminology in counter creation

Some of the terminology in this module is confusing, or has drifted out of sync with other parts of the coverage code.

This PR therefore renames some variables and methods, and adjusts comments and debug logging statements, to make things clearer and more consistent.

No functional changes, other than some small tweaks to debug logging.
This commit is contained in:
Matthias Krüger 2024-09-10 17:35:15 +02:00 committed by GitHub
commit 98222a524d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 102 additions and 132 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

236
compiler/rustc_mir_transform/src/coverage/counters.rs
View File

@ -74,12 +74,11 @@ pub(super) struct CoverageCounters {
} }
impl CoverageCounters { impl CoverageCounters {
/// Makes [`BcbCounter`] `Counter`s and `Expressions` for the `BasicCoverageBlock`s directly or /// Ensures that each BCB node needing a counter has one, by creating physical
/// indirectly associated with coverage spans, and accumulates additional `Expression`s /// counters or counter expressions for nodes and edges as required.
/// representing intermediate values.
pub(super) fn make_bcb_counters( pub(super) fn make_bcb_counters(
basic_coverage_blocks: &CoverageGraph, basic_coverage_blocks: &CoverageGraph,
bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool, bcb_needs_counter: impl Fn(BasicCoverageBlock) -> bool,
) -> Self { ) -> Self {
let num_bcbs = basic_coverage_blocks.num_nodes(); let num_bcbs = basic_coverage_blocks.num_nodes();
@ -91,8 +90,7 @@ pub(super) fn make_bcb_counters(
expressions_memo: FxHashMap::default(), expressions_memo: FxHashMap::default(),
}; };
MakeBcbCounters::new(&mut this, basic_coverage_blocks) MakeBcbCounters::new(&mut this, basic_coverage_blocks).make_bcb_counters(bcb_needs_counter);
.make_bcb_counters(bcb_has_coverage_spans);
this this
} }
@ -241,10 +239,7 @@ pub(super) fn into_expressions(self) -> IndexVec<ExpressionId, Expression> {
} }
} }
/// Traverse the `CoverageGraph` and add either a `Counter` or `Expression` to every BCB, to be /// Helper struct that allows counter creation to inspect the BCB graph.
/// injected with coverage spans. `Expressions` have no runtime overhead, so if a viable expression
/// (adding or subtracting two other counters or expressions) can compute the same result as an
/// embedded counter, an `Expression` should be used.
struct MakeBcbCounters<'a> { struct MakeBcbCounters<'a> {
coverage_counters: &'a mut CoverageCounters, coverage_counters: &'a mut CoverageCounters,
basic_coverage_blocks: &'a CoverageGraph, basic_coverage_blocks: &'a CoverageGraph,
@ -258,36 +253,21 @@ fn new(
Self { coverage_counters, basic_coverage_blocks } Self { coverage_counters, basic_coverage_blocks }
} }
/// If two `BasicCoverageBlock`s branch from another `BasicCoverageBlock`, one of the branches fn make_bcb_counters(&mut self, bcb_needs_counter: impl Fn(BasicCoverageBlock) -> bool) {
/// can be counted by `Expression` by subtracting the other branch from the branching
/// block. Otherwise, the `BasicCoverageBlock` executed the least should have the `Counter`.
/// One way to predict which branch executes the least is by considering loops. A loop is exited
/// at a branch, so the branch that jumps to a `BasicCoverageBlock` outside the loop is almost
/// always executed less than the branch that does not exit the loop.
fn make_bcb_counters(&mut self, bcb_has_coverage_spans: impl Fn(BasicCoverageBlock) -> bool) {
debug!("make_bcb_counters(): adding a counter or expression to each BasicCoverageBlock"); debug!("make_bcb_counters(): adding a counter or expression to each BasicCoverageBlock");
// Walk the `CoverageGraph`. For each `BasicCoverageBlock` node with an associated // Traverse the coverage graph, ensuring that every node that needs a
// coverage span, add a counter. If the `BasicCoverageBlock` branches, add a counter or // coverage counter has one.
// expression to each branch `BasicCoverageBlock` (if the branch BCB has only one incoming
// edge) or edge from the branching BCB to the branch BCB (if the branch BCB has multiple
// incoming edges).
// //
// The `TraverseCoverageGraphWithLoops` traversal ensures that, when a loop is encountered, // The traversal tries to ensure that, when a loop is encountered, all
// all `BasicCoverageBlock` nodes in the loop are visited before visiting any node outside // nodes within the loop are visited before visiting any nodes outside
// the loop. The `traversal` state includes a `context_stack`, providing a way to know if // the loop. It also keeps track of which loop(s) the traversal is
// the current BCB is in one or more nested loops or not. // currently inside.
let mut traversal = TraverseCoverageGraphWithLoops::new(self.basic_coverage_blocks); let mut traversal = TraverseCoverageGraphWithLoops::new(self.basic_coverage_blocks);
while let Some(bcb) = traversal.next() { while let Some(bcb) = traversal.next() {
if bcb_has_coverage_spans(bcb) { let _span = debug_span!("traversal", ?bcb).entered();
debug!("{:?} has at least one coverage span. Get or make its counter", bcb); if bcb_needs_counter(bcb) {
self.make_node_and_branch_counters(&traversal, bcb); self.make_node_counter_and_out_edge_counters(&traversal, bcb);
} else {
debug!(
"{:?} does not have any coverage spans. A counter will only be added if \
and when a covered BCB has an expression dependency.",
bcb,
);
} }
} }
@ -298,73 +278,66 @@ fn make_bcb_counters(&mut self, bcb_has_coverage_spans: impl Fn(BasicCoverageBlo
); );
} }
fn make_node_and_branch_counters( /// Make sure the given node has a node counter, and then make sure each of
/// its out-edges has an edge counter (if appropriate).
#[instrument(level = "debug", skip(self, traversal))]
fn make_node_counter_and_out_edge_counters(
&mut self, &mut self,
traversal: &TraverseCoverageGraphWithLoops<'_>, traversal: &TraverseCoverageGraphWithLoops<'_>,
from_bcb: BasicCoverageBlock, from_bcb: BasicCoverageBlock,
) { ) {
// First, ensure that this node has a counter of some kind. // First, ensure that this node has a counter of some kind.
// We might also use its term later to compute one of the branch counters. // We might also use that counter to compute one of the out-edge counters.
let from_bcb_operand = self.get_or_make_counter_operand(from_bcb); let node_counter = self.get_or_make_node_counter(from_bcb);
let branch_target_bcbs = self.basic_coverage_blocks.successors[from_bcb].as_slice(); let successors = self.basic_coverage_blocks.successors[from_bcb].as_slice();
// If this node doesn't have multiple out-edges, or all of its out-edges // 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. // already have counters, then we don't need to create edge counters.
let needs_branch_counters = branch_target_bcbs.len() > 1 let needs_out_edge_counters = successors.len() > 1
&& branch_target_bcbs && successors.iter().any(|&to_bcb| self.edge_has_no_counter(from_bcb, to_bcb));
.iter() if !needs_out_edge_counters {
.any(|&to_bcb| self.branch_has_no_counter(from_bcb, to_bcb));
if !needs_branch_counters {
return; return;
} }
debug!( if tracing::enabled!(tracing::Level::DEBUG) {
"{from_bcb:?} has some branch(es) without counters:\n {}", let _span =
branch_target_bcbs debug_span!("node has some out-edges without counters", ?from_bcb).entered();
.iter() for &to_bcb in successors {
.map(|&to_bcb| { debug!(?to_bcb, counter=?self.edge_counter(from_bcb, to_bcb));
format!("{from_bcb:?}->{to_bcb:?}: {:?}", self.branch_counter(from_bcb, to_bcb)) }
}) }
.collect::<Vec<_>>()
.join("\n "),
);
// Of the branch edges that don't have counters yet, one can be given an expression // Of the out-edges that don't have counters yet, one can be given an expression
// (computed from the other edges) instead of a dedicated counter. // (computed from the other out-edges) instead of a dedicated counter.
let expression_to_bcb = self.choose_preferred_expression_branch(traversal, from_bcb); let expression_to_bcb = self.choose_out_edge_for_expression(traversal, from_bcb);
// For each branch arm other than the one that was chosen to get an expression, // For each out-edge other than the one that was chosen to get an expression,
// ensure that it has a counter (existing counter/expression or a new counter), // ensure that it has a counter (existing counter/expression or a new counter),
// and accumulate the corresponding terms into a single sum term. // and accumulate the corresponding counters into a single sum expression.
let sum_of_all_other_branches: BcbCounter = { let sum_of_all_other_out_edges: BcbCounter = {
let _span = debug_span!("sum_of_all_other_branches", ?expression_to_bcb).entered(); let _span = debug_span!("sum_of_all_other_out_edges", ?expression_to_bcb).entered();
branch_target_bcbs successors
.iter() .iter()
.copied() .copied()
// Skip the chosen branch, since we'll calculate it from the other branches. // Skip the chosen edge, since we'll calculate its count from this sum.
.filter(|&to_bcb| to_bcb != expression_to_bcb) .filter(|&to_bcb| to_bcb != expression_to_bcb)
.fold(None, |accum, to_bcb| { .fold(None, |accum, to_bcb| {
let _span = debug_span!("to_bcb", ?accum, ?to_bcb).entered(); let _span = debug_span!("to_bcb", ?accum, ?to_bcb).entered();
let branch_counter = self.get_or_make_edge_counter_operand(from_bcb, to_bcb); let edge_counter = self.get_or_make_edge_counter(from_bcb, to_bcb);
Some(self.coverage_counters.make_sum_expression(accum, branch_counter)) Some(self.coverage_counters.make_sum_expression(accum, edge_counter))
}) })
.expect("there must be at least one other branch") .expect("there must be at least one other out-edge")
}; };
// For the branch that was chosen to get an expression, create that expression // Now create an expression for the chosen edge, by taking the counter
// by taking the count of the node we're branching from, and subtracting the // for its source node and subtracting the sum of its sibling out-edges.
// sum of all the other branches.
debug!(
"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( let expression = self.coverage_counters.make_expression(
from_bcb_operand, node_counter,
Op::Subtract, Op::Subtract,
sum_of_all_other_branches, sum_of_all_other_out_edges,
); );
debug!("{expression_to_bcb:?} gets an expression: {expression:?}"); debug!("{expression_to_bcb:?} gets an expression: {expression:?}");
if self.basic_coverage_blocks.bcb_has_multiple_in_edges(expression_to_bcb) { 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); self.coverage_counters.set_bcb_edge_counter(from_bcb, expression_to_bcb, expression);
@ -374,7 +347,7 @@ fn make_node_and_branch_counters(
} }
#[instrument(level = "debug", skip(self))] #[instrument(level = "debug", skip(self))]
fn get_or_make_counter_operand(&mut self, bcb: BasicCoverageBlock) -> BcbCounter { fn get_or_make_node_counter(&mut self, bcb: BasicCoverageBlock) -> BcbCounter {
// If the BCB already has a counter, return it. // If the BCB already has a counter, return it.
if let Some(counter_kind) = self.coverage_counters.bcb_counters[bcb] { if let Some(counter_kind) = self.coverage_counters.bcb_counters[bcb] {
debug!("{bcb:?} already has a counter: {counter_kind:?}"); debug!("{bcb:?} already has a counter: {counter_kind:?}");
@ -411,7 +384,7 @@ fn get_or_make_counter_operand(&mut self, bcb: BasicCoverageBlock) -> BcbCounter
.copied() .copied()
.fold(None, |accum, from_bcb| { .fold(None, |accum, from_bcb| {
let _span = debug_span!("from_bcb", ?accum, ?from_bcb).entered(); let _span = debug_span!("from_bcb", ?accum, ?from_bcb).entered();
let edge_counter = self.get_or_make_edge_counter_operand(from_bcb, bcb); let edge_counter = self.get_or_make_edge_counter(from_bcb, bcb);
Some(self.coverage_counters.make_sum_expression(accum, edge_counter)) Some(self.coverage_counters.make_sum_expression(accum, edge_counter))
}) })
.expect("there must be at least one in-edge") .expect("there must be at least one in-edge")
@ -422,7 +395,7 @@ fn get_or_make_counter_operand(&mut self, bcb: BasicCoverageBlock) -> BcbCounter
} }
#[instrument(level = "debug", skip(self))] #[instrument(level = "debug", skip(self))]
fn get_or_make_edge_counter_operand( fn get_or_make_edge_counter(
&mut self, &mut self,
from_bcb: BasicCoverageBlock, from_bcb: BasicCoverageBlock,
to_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock,
@ -431,13 +404,13 @@ fn get_or_make_edge_counter_operand(
// a node counter instead, since it will have the same value. // a node counter instead, since it will have the same value.
if !self.basic_coverage_blocks.bcb_has_multiple_in_edges(to_bcb) { 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]); assert_eq!([from_bcb].as_slice(), self.basic_coverage_blocks.predecessors[to_bcb]);
return self.get_or_make_counter_operand(to_bcb); return self.get_or_make_node_counter(to_bcb);
} }
// If the source BCB has only one successor (assumed to be the given target), an edge // 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. // counter is unnecessary. Just get or make a counter for the source BCB.
if self.bcb_successors(from_bcb).len() == 1 { if self.bcb_successors(from_bcb).len() == 1 {
return self.get_or_make_counter_operand(from_bcb); return self.get_or_make_node_counter(from_bcb);
} }
// If the edge already has a counter, return it. // If the edge already has a counter, return it.
@ -455,82 +428,79 @@ fn get_or_make_edge_counter_operand(
self.coverage_counters.set_bcb_edge_counter(from_bcb, to_bcb, counter_kind) self.coverage_counters.set_bcb_edge_counter(from_bcb, to_bcb, counter_kind)
} }
/// Select a branch for the expression, either the recommended `reloop_branch`, or if none was /// Choose one of the out-edges of `from_bcb` to receive an expression
/// found, select any branch. /// instead of a physical counter, and returns that edge's target node.
fn choose_preferred_expression_branch( ///
/// - Precondition: The node must have at least one out-edge without a counter.
/// - Postcondition: The selected edge does not have an edge counter.
fn choose_out_edge_for_expression(
&self, &self,
traversal: &TraverseCoverageGraphWithLoops<'_>, traversal: &TraverseCoverageGraphWithLoops<'_>,
from_bcb: BasicCoverageBlock, from_bcb: BasicCoverageBlock,
) -> BasicCoverageBlock { ) -> BasicCoverageBlock {
let good_reloop_branch = self.find_good_reloop_branch(traversal, from_bcb); if let Some(reloop_target) = self.find_good_reloop_edge(traversal, from_bcb) {
if let Some(reloop_target) = good_reloop_branch { assert!(self.edge_has_no_counter(from_bcb, reloop_target));
assert!(self.branch_has_no_counter(from_bcb, reloop_target));
debug!("Selecting reloop target {reloop_target:?} to get an expression"); debug!("Selecting reloop target {reloop_target:?} to get an expression");
reloop_target return reloop_target;
} else {
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",
);
debug!(
"Selecting any branch={:?} that still needs a counter, to get the \
`Expression` because there was no `reloop_branch`, or it already had a \
counter",
branch_without_counter
);
branch_without_counter
}
} }
/// Tries to find a branch that leads back to the top of a loop, and that // We couldn't identify a "good" edge, so just choose any edge that
/// doesn't already have a counter. Such branches are good candidates to // doesn't already have a counter.
let arbitrary_target = self
.bcb_successors(from_bcb)
.iter()
.copied()
.find(|&to_bcb| self.edge_has_no_counter(from_bcb, to_bcb))
.expect("precondition: at least one out-edge without a counter");
debug!(?arbitrary_target, "selecting arbitrary out-edge to get an expression");
arbitrary_target
}
/// Tries to find an edge that leads back to the top of a loop, and that
/// doesn't already have a counter. Such edges are good candidates to
/// be given an expression (instead of a physical counter), because they /// be given an expression (instead of a physical counter), because they
/// will tend to be executed more times than a loop-exit branch. /// will tend to be executed more times than a loop-exit edge.
fn find_good_reloop_branch( fn find_good_reloop_edge(
&self, &self,
traversal: &TraverseCoverageGraphWithLoops<'_>, traversal: &TraverseCoverageGraphWithLoops<'_>,
from_bcb: BasicCoverageBlock, from_bcb: BasicCoverageBlock,
) -> Option<BasicCoverageBlock> { ) -> Option<BasicCoverageBlock> {
let branch_target_bcbs = self.bcb_successors(from_bcb); let successors = self.bcb_successors(from_bcb);
// Consider each loop on the current traversal context stack, top-down. // Consider each loop on the current traversal context stack, top-down.
for reloop_bcbs in traversal.reloop_bcbs_per_loop() { for reloop_bcbs in traversal.reloop_bcbs_per_loop() {
let mut all_branches_exit_this_loop = true; let mut all_edges_exit_this_loop = true;
// Try to find a branch that doesn't exit this loop and doesn't // Try to find an out-edge that doesn't exit this loop and doesn't
// already have a counter. // already have a counter.
for &branch_target_bcb in branch_target_bcbs { for &target_bcb in successors {
// A branch is a reloop branch if it dominates any BCB that has // An edge is a reloop edge if its target dominates any BCB that has
// an edge back to the loop header. (Other branches are exits.) // an edge back to the loop header. (Otherwise it's an exit edge.)
let is_reloop_branch = reloop_bcbs.iter().any(|&reloop_bcb| { let is_reloop_edge = reloop_bcbs.iter().any(|&reloop_bcb| {
self.basic_coverage_blocks.dominates(branch_target_bcb, reloop_bcb) self.basic_coverage_blocks.dominates(target_bcb, reloop_bcb)
}); });
if is_reloop_branch { if is_reloop_edge {
all_branches_exit_this_loop = false; all_edges_exit_this_loop = false;
if self.branch_has_no_counter(from_bcb, branch_target_bcb) { if self.edge_has_no_counter(from_bcb, target_bcb) {
// We found a good branch to be given an expression. // We found a good out-edge to be given an expression.
return Some(branch_target_bcb); return Some(target_bcb);
} }
// Keep looking for another reloop branch without a counter. // Keep looking for another reloop edge without a counter.
} else { } else {
// This branch exits the loop. // This edge exits the loop.
} }
} }
if !all_branches_exit_this_loop { if !all_edges_exit_this_loop {
// We found one or more reloop branches, but all of them already // We found one or more reloop edges, but all of them already
// have counters. Let the caller choose one of the exit branches. // have counters. Let the caller choose one of the other edges.
debug!("All reloop branches had counters; skip checking the other loops"); debug!("All reloop edges had counters; skipping the other loops");
return None; return None;
} }
// All of the branches exit this loop, so keep looking for a good // All of the out-edges exit this loop, so keep looking for a good
// reloop branch for one of the outer loops. // reloop edge for one of the outer loops.
} }
None None
@ -547,15 +517,15 @@ fn bcb_successors(&self, bcb: BasicCoverageBlock) -> &[BasicCoverageBlock] {
} }
#[inline] #[inline]
fn branch_has_no_counter( fn edge_has_no_counter(
&self, &self,
from_bcb: BasicCoverageBlock, from_bcb: BasicCoverageBlock,
to_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock,
) -> bool { ) -> bool {
self.branch_counter(from_bcb, to_bcb).is_none() self.edge_counter(from_bcb, to_bcb).is_none()
} }
fn branch_counter( fn edge_counter(
&self, &self,
from_bcb: BasicCoverageBlock, from_bcb: BasicCoverageBlock,
to_bcb: BasicCoverageBlock, to_bcb: BasicCoverageBlock,