Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

coverage: Remove some complex heuristics from counter creation #131398

Closed
wants to merge 3 commits into from
Closed

coverage: Remove some complex heuristics from counter creation #131398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7d271a
coverage: Include the highest counter ID seen in `.cov-map` dumps
Zalathar Oct 8, 2024
f432e15
coverage: Remove the loop-based heuristic for choosing expression edges
Zalathar Oct 8, 2024
9e675b1
coverage: Remove the custom graph traversal from counter creation
Zalathar Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 10 additions & 74 deletions compiler/rustc_mir_transform/src/coverage/counters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ use rustc_index::IndexVec;
use rustc_index::bit_set::BitSet;
use rustc_middle::bug;
use rustc_middle::mir::coverage::{CounterId, CovTerm, Expression, ExpressionId, Op};
use tracing::{debug, debug_span, instrument};
use tracing::{debug, instrument};

use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph, TraverseCoverageGraphWithLoops};
use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph};

/// The coverage counter or counter expression associated with a particular
/// BCB node or BCB edge.
Expand Down Expand Up @@ -290,34 +290,15 @@ impl<'a> MakeBcbCounters<'a> {

// Traverse the coverage graph, ensuring that every node that needs a
// coverage counter has one.
//
// The traversal tries to ensure that, when a loop is encountered, all
// nodes within the loop are visited before visiting any nodes outside
// the loop. It also keeps track of which loop(s) the traversal is
// currently inside.
let mut traversal = TraverseCoverageGraphWithLoops::new(self.basic_coverage_blocks);
while let Some(bcb) = traversal.next() {
let _span = debug_span!("traversal", ?bcb).entered();
if self.bcb_needs_counter.contains(bcb) {
self.make_node_counter_and_out_edge_counters(&traversal, bcb);
}
for bcb in self.bcb_needs_counter.iter() {
self.make_node_counter_and_out_edge_counters(bcb);
}

assert!(
traversal.is_complete(),
"`TraverseCoverageGraphWithLoops` missed some `BasicCoverageBlock`s: {:?}",
traversal.unvisited(),
);
}

/// 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,
traversal: &TraverseCoverageGraphWithLoops<'_>,
from_bcb: BasicCoverageBlock,
) {
#[instrument(level = "debug", skip(self))]
fn make_node_counter_and_out_edge_counters(&mut self, from_bcb: BasicCoverageBlock) {
// First, ensure that this node has a counter of some kind.
// We might also use that counter to compute one of the out-edge counters.
let node_counter = self.get_or_make_node_counter(from_bcb);
Expand All @@ -340,8 +321,7 @@ impl<'a> MakeBcbCounters<'a> {

// If there are out-edges without counters, choose one to be given an expression
// (computed from this node and the other out-edges) instead of a physical counter.
let Some(expression_to_bcb) =
self.choose_out_edge_for_expression(traversal, &candidate_successors)
let Some(expression_to_bcb) = self.choose_out_edge_for_expression(&candidate_successors)
else {
return;
};
Expand Down Expand Up @@ -455,60 +435,16 @@ impl<'a> MakeBcbCounters<'a> {
/// choose one to be given a counter expression instead of a physical counter.
fn choose_out_edge_for_expression(
&self,
traversal: &TraverseCoverageGraphWithLoops<'_>,
candidate_successors: &[BasicCoverageBlock],
) -> Option<BasicCoverageBlock> {
// Try to find a candidate that leads back to the top of a loop,
// because reloop edges tend to be executed more times than loop-exit edges.
if let Some(reloop_target) = self.find_good_reloop_edge(traversal, &candidate_successors) {
debug!("Selecting reloop target {reloop_target:?} to get an expression");
return Some(reloop_target);
}

// We couldn't identify a "good" edge, so just choose an arbitrary one.
// For now, just choose an arbitrary edge.
// FIXME(Zalathar): This was greatly simplified to make other refactoring
// easier, but eventually it might be good to make this smarter again.
let arbitrary_target = candidate_successors.first().copied()?;
debug!(?arbitrary_target, "selecting arbitrary out-edge to get an expression");
Some(arbitrary_target)
}

/// Given a set of candidate out-edges (represented by their successor node),
/// tries to find one that leads back to the top of a loop.
///
/// Reloop edges are good candidates for counter expressions, because they
/// will tend to be executed more times than a loop-exit edge, so it's nice
/// for them to be able to avoid a physical counter increment.
fn find_good_reloop_edge(
&self,
traversal: &TraverseCoverageGraphWithLoops<'_>,
candidate_successors: &[BasicCoverageBlock],
) -> Option<BasicCoverageBlock> {
// If there are no candidates, avoid iterating over the loop stack.
if candidate_successors.is_empty() {
return None;
}

// Consider each loop on the current traversal context stack, top-down.
for reloop_bcbs in traversal.reloop_bcbs_per_loop() {
// Try to find a candidate edge that doesn't exit this loop.
for &target_bcb in candidate_successors {
// An edge is a reloop edge if its target dominates any BCB that has
// an edge back to the loop header. (Otherwise it's an exit edge.)
let is_reloop_edge = reloop_bcbs.iter().any(|&reloop_bcb| {
self.basic_coverage_blocks.dominates(target_bcb, reloop_bcb)
});
if is_reloop_edge {
// We found a good out-edge to be given an expression.
return Some(target_bcb);
}
}

// All of the candidate edges exit this loop, so keep looking
// for a good reloop edge for one of the outer loops.
}

None
}

#[inline]
fn edge_has_no_counter(
&self,
Expand Down
177 changes: 1 addition & 176 deletions compiler/rustc_mir_transform/src/coverage/graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
use std::cmp::Ordering;
use std::collections::VecDeque;
use std::ops::{Index, IndexMut};

use rustc_data_structures::captures::Captures;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::graph;
use rustc_data_structures::graph::dominators::{self, Dominators};
use rustc_data_structures::graph::{self, DirectedGraph, StartNode};
use rustc_index::IndexVec;
use rustc_index::bit_set::BitSet;
use rustc_middle::bug;
use rustc_middle::mir::{self, BasicBlock, Terminator, TerminatorKind};
use tracing::debug;

Expand Down Expand Up @@ -151,11 +149,6 @@ impl CoverageGraph {
if bb.index() < self.bb_to_bcb.len() { self.bb_to_bcb[bb] } else { None }
}

#[inline(always)]
pub(crate) fn dominates(&self, dom: BasicCoverageBlock, node: BasicCoverageBlock) -> bool {
self.dominators.as_ref().unwrap().dominates(dom, node)
}

#[inline(always)]
pub(crate) fn cmp_in_dominator_order(
&self,
Expand Down Expand Up @@ -400,174 +393,6 @@ fn bcb_filtered_successors<'a, 'tcx>(terminator: &'a Terminator<'tcx>) -> Covera
}
}

/// Maintains separate worklists for each loop in the BasicCoverageBlock CFG, plus one for the
/// CoverageGraph outside all loops. This supports traversing the BCB CFG in a way that
/// ensures a loop is completely traversed before processing Blocks after the end of the loop.
#[derive(Debug)]
struct TraversalContext {
/// BCB with one or more incoming loop backedges, indicating which loop
/// this context is for.
///
/// If `None`, this is the non-loop context for the function as a whole.
loop_header: Option<BasicCoverageBlock>,

/// Worklist of BCBs to be processed in this context.
worklist: VecDeque<BasicCoverageBlock>,
}

pub(crate) struct TraverseCoverageGraphWithLoops<'a> {
basic_coverage_blocks: &'a CoverageGraph,

backedges: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>,
context_stack: Vec<TraversalContext>,
visited: BitSet<BasicCoverageBlock>,
}

impl<'a> TraverseCoverageGraphWithLoops<'a> {
pub(crate) fn new(basic_coverage_blocks: &'a CoverageGraph) -> Self {
let backedges = find_loop_backedges(basic_coverage_blocks);

let worklist = VecDeque::from([basic_coverage_blocks.start_node()]);
let context_stack = vec![TraversalContext { loop_header: None, worklist }];

// `context_stack` starts with a `TraversalContext` for the main function context (beginning
// with the `start` BasicCoverageBlock of the function). New worklists are pushed to the top
// of the stack as loops are entered, and popped off of the stack when a loop's worklist is
// exhausted.
let visited = BitSet::new_empty(basic_coverage_blocks.num_nodes());
Self { basic_coverage_blocks, backedges, context_stack, visited }
}

/// For each loop on the loop context stack (top-down), yields a list of BCBs
/// within that loop that have an outgoing edge back to the loop header.
pub(crate) fn reloop_bcbs_per_loop(&self) -> impl Iterator<Item = &[BasicCoverageBlock]> {
self.context_stack
.iter()
.rev()
.filter_map(|context| context.loop_header)
.map(|header_bcb| self.backedges[header_bcb].as_slice())
}

pub(crate) fn next(&mut self) -> Option<BasicCoverageBlock> {
debug!(
"TraverseCoverageGraphWithLoops::next - context_stack: {:?}",
self.context_stack.iter().rev().collect::<Vec<_>>()
);

while let Some(context) = self.context_stack.last_mut() {
let Some(bcb) = context.worklist.pop_front() else {
// This stack level is exhausted; pop it and try the next one.
self.context_stack.pop();
continue;
};

if !self.visited.insert(bcb) {
debug!("Already visited: {bcb:?}");
continue;
}
debug!("Visiting {bcb:?}");

if self.backedges[bcb].len() > 0 {
debug!("{bcb:?} is a loop header! Start a new TraversalContext...");
self.context_stack
.push(TraversalContext { loop_header: Some(bcb), worklist: VecDeque::new() });
}
self.add_successors_to_worklists(bcb);
return Some(bcb);
}

None
}

fn add_successors_to_worklists(&mut self, bcb: BasicCoverageBlock) {
let successors = &self.basic_coverage_blocks.successors[bcb];
debug!("{:?} has {} successors:", bcb, successors.len());

for &successor in successors {
if successor == bcb {
debug!(
"{:?} has itself as its own successor. (Note, the compiled code will \
generate an infinite loop.)",
bcb
);
// Don't re-add this successor to the worklist. We are already processing it.
// FIXME: This claims to skip just the self-successor, but it actually skips
// all other successors as well. Does that matter?
break;
}

// Add successors of the current BCB to the appropriate context. Successors that
// stay within a loop are added to the BCBs context worklist. Successors that
// exit the loop (they are not dominated by the loop header) must be reachable
// from other BCBs outside the loop, and they will be added to a different
// worklist.
//
// Branching blocks (with more than one successor) must be processed before
// blocks with only one successor, to prevent unnecessarily complicating
// `Expression`s by creating a Counter in a `BasicCoverageBlock` that the
// branching block would have given an `Expression` (or vice versa).

let context = self
.context_stack
.iter_mut()
.rev()
.find(|context| match context.loop_header {
Some(loop_header) => {
self.basic_coverage_blocks.dominates(loop_header, successor)
}
None => true,
})
.unwrap_or_else(|| bug!("should always fall back to the root non-loop context"));
debug!("adding to worklist for {:?}", context.loop_header);

// FIXME: The code below had debug messages claiming to add items to a
// particular end of the worklist, but was confused about which end was
// which. The existing behaviour has been preserved for now, but it's
// unclear what the intended behaviour was.

if self.basic_coverage_blocks.successors[successor].len() > 1 {
context.worklist.push_back(successor);
} else {
context.worklist.push_front(successor);
}
}
}

pub(crate) fn is_complete(&self) -> bool {
self.visited.count() == self.visited.domain_size()
}

pub(crate) fn unvisited(&self) -> Vec<BasicCoverageBlock> {
let mut unvisited_set: BitSet<BasicCoverageBlock> =
BitSet::new_filled(self.visited.domain_size());
unvisited_set.subtract(&self.visited);
unvisited_set.iter().collect::<Vec<_>>()
}
}

fn find_loop_backedges(
basic_coverage_blocks: &CoverageGraph,
) -> IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>> {
let num_bcbs = basic_coverage_blocks.num_nodes();
let mut backedges = IndexVec::from_elem_n(Vec::<BasicCoverageBlock>::new(), num_bcbs);

// Identify loops by their backedges.
for (bcb, _) in basic_coverage_blocks.iter_enumerated() {
for &successor in &basic_coverage_blocks.successors[bcb] {
if basic_coverage_blocks.dominates(successor, bcb) {
let loop_header = successor;
let backedge_from_bcb = bcb;
debug!(
"Found BCB backedge: {:?} -> loop_header: {:?}",
backedge_from_bcb, loop_header
);
backedges[loop_header].push(backedge_from_bcb);
}
}
}
backedges
}

fn short_circuit_preorder<'a, 'tcx, F, Iter>(
body: &'a mir::Body<'tcx>,
filtered_successors: F,
Expand Down
36 changes: 36 additions & 0 deletions src/tools/coverage-dump/src/covfun.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,19 @@ pub(crate) fn dump_covfun_mappings(
expression_resolver.push_operands(lhs, rhs);
}

let mut max_counter = None;
for i in 0..num_files {
let num_mappings = parser.read_uleb128_u32()?;
println!("Number of file {i} mappings: {num_mappings}");

for _ in 0..num_mappings {
let (kind, region) = parser.read_mapping_kind_and_region()?;
println!("- {kind:?} at {region:?}");
kind.for_each_term(|term| {
if let CovTerm::Counter(n) = term {
max_counter = max_counter.max(Some(n));
}
});

match kind {
// Also print expression mappings in resolved form.
Expand All @@ -83,6 +89,10 @@ pub(crate) fn dump_covfun_mappings(
}

parser.ensure_empty()?;
println!("Max counter seen: {}", match max_counter {
Some(id) => format!("c{id}"),
None => "(none)".to_owned(),
});
println!();
}
Ok(())
Expand Down Expand Up @@ -271,6 +281,32 @@ enum MappingKind {
},
}

impl MappingKind {
fn for_each_term(&self, mut callback: impl FnMut(CovTerm)) {
match *self {
Self::Code(term) => callback(term),
Self::Gap(term) => callback(term),
Self::Expansion(_id) => {}
Self::Skip => {}
Self::Branch { r#true, r#false } => {
callback(r#true);
callback(r#false);
}
Self::MCDCBranch {
r#true,
r#false,
condition_id: _,
true_next_id: _,
false_next_id: _,
} => {
callback(r#true);
callback(r#false);
}
Self::MCDCDecision { bitmap_idx: _, conditions_num: _ } => {}
}
}
}

struct MappingRegion {
/// Offset of this region's start line, relative to the *start line* of
/// the *previous mapping* (or 0). Line numbers are 1-based.
Expand Down
Loading
Loading