Skip to content

Commit f5541e0

Browse files
committed
coverage: Prefer to visit nodes whose predecessors have been visited
1 parent 706141b commit f5541e0

25 files changed

+908
-1222
lines changed

compiler/rustc_mir_transform/src/coverage/counters.rs

+2-13
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use rustc_index::bit_set::BitSet;
99
use rustc_middle::mir::coverage::{CounterId, CovTerm, Expression, ExpressionId, Op};
1010
use tracing::{debug, debug_span, instrument};
1111

12-
use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph, TraverseCoverageGraphWithLoops};
12+
use crate::coverage::graph::{BasicCoverageBlock, CoverageGraph, ReadyFirstTraversal};
1313

1414
#[cfg(test)]
1515
mod tests;
@@ -236,23 +236,12 @@ impl<'a> CountersBuilder<'a> {
236236

237237
// Traverse the coverage graph, ensuring that every node that needs a
238238
// coverage counter has one.
239-
//
240-
// The traversal tries to ensure that, when a loop is encountered, all
241-
// nodes within the loop are visited before visiting any nodes outside
242-
// the loop.
243-
let mut traversal = TraverseCoverageGraphWithLoops::new(self.graph);
244-
while let Some(bcb) = traversal.next() {
239+
for bcb in ReadyFirstTraversal::new(self.graph) {
245240
let _span = debug_span!("traversal", ?bcb).entered();
246241
if self.bcb_needs_counter.contains(bcb) {
247242
self.make_node_counter_and_out_edge_counters(bcb);
248243
}
249244
}
250-
251-
assert!(
252-
traversal.is_complete(),
253-
"`TraverseCoverageGraphWithLoops` missed some `BasicCoverageBlock`s: {:?}",
254-
traversal.unvisited(),
255-
);
256245
}
257246

258247
/// Make sure the given node has a node counter, and then make sure each of

compiler/rustc_mir_transform/src/coverage/graph.rs

+123-133
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ use rustc_data_structures::graph::dominators::Dominators;
99
use rustc_data_structures::graph::{self, DirectedGraph, StartNode};
1010
use rustc_index::IndexVec;
1111
use rustc_index::bit_set::BitSet;
12-
use rustc_middle::bug;
1312
use rustc_middle::mir::{self, BasicBlock, Terminator, TerminatorKind};
1413
use tracing::debug;
1514

@@ -462,138 +461,6 @@ fn bcb_filtered_successors<'a, 'tcx>(terminator: &'a Terminator<'tcx>) -> Covera
462461
CoverageSuccessors { targets, is_yield }
463462
}
464463

465-
/// Maintains separate worklists for each loop in the BasicCoverageBlock CFG, plus one for the
466-
/// CoverageGraph outside all loops. This supports traversing the BCB CFG in a way that
467-
/// ensures a loop is completely traversed before processing Blocks after the end of the loop.
468-
#[derive(Debug)]
469-
struct TraversalContext {
470-
/// BCB with one or more incoming loop backedges, indicating which loop
471-
/// this context is for.
472-
///
473-
/// If `None`, this is the non-loop context for the function as a whole.
474-
loop_header: Option<BasicCoverageBlock>,
475-
476-
/// Worklist of BCBs to be processed in this context.
477-
worklist: VecDeque<BasicCoverageBlock>,
478-
}
479-
480-
pub(crate) struct TraverseCoverageGraphWithLoops<'a> {
481-
basic_coverage_blocks: &'a CoverageGraph,
482-
483-
context_stack: Vec<TraversalContext>,
484-
visited: BitSet<BasicCoverageBlock>,
485-
}
486-
487-
impl<'a> TraverseCoverageGraphWithLoops<'a> {
488-
pub(crate) fn new(basic_coverage_blocks: &'a CoverageGraph) -> Self {
489-
let worklist = VecDeque::from([basic_coverage_blocks.start_node()]);
490-
let context_stack = vec![TraversalContext { loop_header: None, worklist }];
491-
492-
// `context_stack` starts with a `TraversalContext` for the main function context (beginning
493-
// with the `start` BasicCoverageBlock of the function). New worklists are pushed to the top
494-
// of the stack as loops are entered, and popped off of the stack when a loop's worklist is
495-
// exhausted.
496-
let visited = BitSet::new_empty(basic_coverage_blocks.num_nodes());
497-
Self { basic_coverage_blocks, context_stack, visited }
498-
}
499-
500-
pub(crate) fn next(&mut self) -> Option<BasicCoverageBlock> {
501-
debug!(
502-
"TraverseCoverageGraphWithLoops::next - context_stack: {:?}",
503-
self.context_stack.iter().rev().collect::<Vec<_>>()
504-
);
505-
506-
while let Some(context) = self.context_stack.last_mut() {
507-
let Some(bcb) = context.worklist.pop_front() else {
508-
// This stack level is exhausted; pop it and try the next one.
509-
self.context_stack.pop();
510-
continue;
511-
};
512-
513-
if !self.visited.insert(bcb) {
514-
debug!("Already visited: {bcb:?}");
515-
continue;
516-
}
517-
debug!("Visiting {bcb:?}");
518-
519-
if self.basic_coverage_blocks.is_loop_header.contains(bcb) {
520-
debug!("{bcb:?} is a loop header! Start a new TraversalContext...");
521-
self.context_stack
522-
.push(TraversalContext { loop_header: Some(bcb), worklist: VecDeque::new() });
523-
}
524-
self.add_successors_to_worklists(bcb);
525-
return Some(bcb);
526-
}
527-
528-
None
529-
}
530-
531-
fn add_successors_to_worklists(&mut self, bcb: BasicCoverageBlock) {
532-
let successors = &self.basic_coverage_blocks.successors[bcb];
533-
debug!("{:?} has {} successors:", bcb, successors.len());
534-
535-
for &successor in successors {
536-
if successor == bcb {
537-
debug!(
538-
"{:?} has itself as its own successor. (Note, the compiled code will \
539-
generate an infinite loop.)",
540-
bcb
541-
);
542-
// Don't re-add this successor to the worklist. We are already processing it.
543-
// FIXME: This claims to skip just the self-successor, but it actually skips
544-
// all other successors as well. Does that matter?
545-
break;
546-
}
547-
548-
// Add successors of the current BCB to the appropriate context. Successors that
549-
// stay within a loop are added to the BCBs context worklist. Successors that
550-
// exit the loop (they are not dominated by the loop header) must be reachable
551-
// from other BCBs outside the loop, and they will be added to a different
552-
// worklist.
553-
//
554-
// Branching blocks (with more than one successor) must be processed before
555-
// blocks with only one successor, to prevent unnecessarily complicating
556-
// `Expression`s by creating a Counter in a `BasicCoverageBlock` that the
557-
// branching block would have given an `Expression` (or vice versa).
558-
559-
let context = self
560-
.context_stack
561-
.iter_mut()
562-
.rev()
563-
.find(|context| match context.loop_header {
564-
Some(loop_header) => {
565-
self.basic_coverage_blocks.dominates(loop_header, successor)
566-
}
567-
None => true,
568-
})
569-
.unwrap_or_else(|| bug!("should always fall back to the root non-loop context"));
570-
debug!("adding to worklist for {:?}", context.loop_header);
571-
572-
// FIXME: The code below had debug messages claiming to add items to a
573-
// particular end of the worklist, but was confused about which end was
574-
// which. The existing behaviour has been preserved for now, but it's
575-
// unclear what the intended behaviour was.
576-
577-
if self.basic_coverage_blocks.successors[successor].len() > 1 {
578-
context.worklist.push_back(successor);
579-
} else {
580-
context.worklist.push_front(successor);
581-
}
582-
}
583-
}
584-
585-
pub(crate) fn is_complete(&self) -> bool {
586-
self.visited.count() == self.visited.domain_size()
587-
}
588-
589-
pub(crate) fn unvisited(&self) -> Vec<BasicCoverageBlock> {
590-
let mut unvisited_set: BitSet<BasicCoverageBlock> =
591-
BitSet::new_filled(self.visited.domain_size());
592-
unvisited_set.subtract(&self.visited);
593-
unvisited_set.iter().collect::<Vec<_>>()
594-
}
595-
}
596-
597464
/// Wrapper around a [`mir::BasicBlocks`] graph that restricts each node's
598465
/// successors to only the ones considered "relevant" when building a coverage
599466
/// graph.
@@ -622,3 +489,126 @@ impl<'a, 'tcx> graph::Successors for CoverageRelevantSubgraph<'a, 'tcx> {
622489
self.coverage_successors(bb).into_iter()
623490
}
624491
}
492+
493+
/// State of a node in the coverage graph during ready-first traversal.
494+
#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
495+
enum ReadyState {
496+
/// This node has not yet been added to the fallback queue or ready queue.
497+
Unqueued,
498+
/// This node is currently in the fallback queue.
499+
InFallbackQueue,
500+
/// This node's predecessors have all been visited, so it is in the ready queue.
501+
/// (It might also have a stale entry in the fallback queue.)
502+
InReadyQueue,
503+
/// This node has been visited.
504+
/// (It might also have a stale entry in the fallback queue.)
505+
Visited,
506+
}
507+
508+
/// Iterator that visits nodes in the coverage graph, in an order that always
509+
/// prefers "ready" nodes whose predecessors have already been visited.
510+
pub(crate) struct ReadyFirstTraversal<'a> {
511+
graph: &'a CoverageGraph,
512+
513+
/// For each node, the number of its predecessor nodes that haven't been visited yet.
514+
n_unvisited_preds: IndexVec<BasicCoverageBlock, u32>,
515+
/// Indicates whether a node has been visited, or which queue it is in.
516+
state: IndexVec<BasicCoverageBlock, ReadyState>,
517+
518+
/// Holds unvisited nodes whose predecessors have all been visited.
519+
ready_queue: VecDeque<BasicCoverageBlock>,
520+
/// Holds unvisited nodes with some unvisited predecessors.
521+
/// Also contains stale entries for nodes that were upgraded to ready.
522+
fallback_queue: VecDeque<BasicCoverageBlock>,
523+
}
524+
525+
impl<'a> ReadyFirstTraversal<'a> {
526+
pub(crate) fn new(graph: &'a CoverageGraph) -> Self {
527+
let num_nodes = graph.num_nodes();
528+
529+
let n_unvisited_preds =
530+
IndexVec::from_fn_n(|node| graph.predecessors[node].len() as u32, num_nodes);
531+
let mut state = IndexVec::from_elem_n(ReadyState::Unqueued, num_nodes);
532+
533+
// We know from coverage graph construction that the start node is the
534+
// only node with no predecessors.
535+
debug_assert!(
536+
n_unvisited_preds.iter_enumerated().all(|(node, &n)| (node == START_BCB) == (n == 0))
537+
);
538+
let ready_queue = VecDeque::from(vec![START_BCB]);
539+
state[START_BCB] = ReadyState::InReadyQueue;
540+
541+
Self { graph, state, n_unvisited_preds, ready_queue, fallback_queue: VecDeque::new() }
542+
}
543+
544+
/// Returns the next node from the ready queue, or else the next unvisited
545+
/// node from the fallback queue.
546+
fn next_inner(&mut self) -> Option<BasicCoverageBlock> {
547+
// Always prefer to yield a ready node if possible.
548+
if let Some(node) = self.ready_queue.pop_front() {
549+
assert_eq!(self.state[node], ReadyState::InReadyQueue);
550+
return Some(node);
551+
}
552+
553+
while let Some(node) = self.fallback_queue.pop_front() {
554+
match self.state[node] {
555+
// This entry in the fallback queue is not stale, so yield it.
556+
ReadyState::InFallbackQueue => return Some(node),
557+
// This node was added to the fallback queue, but later became
558+
// ready and was visited via the ready queue. Ignore it here.
559+
ReadyState::Visited => {}
560+
// Unqueued nodes can't be in the fallback queue, by definition.
561+
// We know that the ready queue is empty at this point.
562+
ReadyState::Unqueued | ReadyState::InReadyQueue => unreachable!(
563+
"unexpected state for {node:?} in the fallback queue: {:?}",
564+
self.state[node]
565+
),
566+
}
567+
}
568+
569+
None
570+
}
571+
572+
fn mark_visited_and_enqueue_successors(&mut self, node: BasicCoverageBlock) {
573+
assert!(self.state[node] < ReadyState::Visited);
574+
self.state[node] = ReadyState::Visited;
575+
576+
// For each of this node's successors, decrease the successor's
577+
// "unvisited predecessors" count, and enqueue it if appropriate.
578+
for &succ in &self.graph.successors[node] {
579+
let is_unqueued = match self.state[succ] {
580+
ReadyState::Unqueued => true,
581+
ReadyState::InFallbackQueue => false,
582+
ReadyState::InReadyQueue => {
583+
unreachable!("nodes in the ready queue have no unvisited predecessors")
584+
}
585+
// The successor was already visited via one of its other predecessors.
586+
ReadyState::Visited => continue,
587+
};
588+
589+
self.n_unvisited_preds[succ] -= 1;
590+
if self.n_unvisited_preds[succ] == 0 {
591+
// This node's predecessors have all been visited, so add it to
592+
// the ready queue. If it's already in the fallback queue, that
593+
// fallback entry will be ignored later.
594+
self.state[succ] = ReadyState::InReadyQueue;
595+
self.ready_queue.push_back(succ);
596+
} else if is_unqueued {
597+
// This node has unvisited predecessors, so add it to the
598+
// fallback queue in case we run out of ready nodes later.
599+
self.state[succ] = ReadyState::InFallbackQueue;
600+
self.fallback_queue.push_back(succ);
601+
}
602+
}
603+
}
604+
}
605+
606+
impl<'a> Iterator for ReadyFirstTraversal<'a> {
607+
type Item = BasicCoverageBlock;
608+
609+
fn next(&mut self) -> Option<Self::Item> {
610+
let node = self.next_inner()?;
611+
self.mark_visited_and_enqueue_successors(node);
612+
Some(node)
613+
}
614+
}

tests/coverage/branch/guard.cov-map

+21-19
Original file line numberDiff line numberDiff line change
@@ -1,35 +1,37 @@
11
Function name: guard::branch_match_guard
2-
Raw bytes (85): 0x[01, 01, 06, 19, 0d, 05, 09, 0f, 15, 13, 11, 17, 0d, 05, 09, 0d, 01, 0c, 01, 01, 10, 02, 03, 0b, 00, 0c, 15, 01, 14, 02, 0a, 0d, 03, 0e, 00, 0f, 19, 00, 14, 00, 19, 20, 0d, 02, 00, 14, 00, 1e, 0d, 00, 1d, 02, 0a, 11, 03, 0e, 00, 0f, 02, 00, 14, 00, 19, 20, 11, 05, 00, 14, 00, 1e, 11, 00, 1d, 02, 0a, 17, 03, 0e, 02, 0a, 0b, 04, 01, 00, 02]
2+
Raw bytes (89): 0x[01, 01, 08, 05, 0d, 05, 17, 0d, 11, 1f, 17, 05, 09, 0d, 11, 1f, 15, 05, 09, 0d, 01, 0c, 01, 01, 10, 02, 03, 0b, 00, 0c, 15, 01, 14, 02, 0a, 0d, 03, 0e, 00, 0f, 05, 00, 14, 00, 19, 20, 0d, 02, 00, 14, 00, 1e, 0d, 00, 1d, 02, 0a, 11, 03, 0e, 00, 0f, 02, 00, 14, 00, 19, 20, 11, 06, 00, 14, 00, 1e, 11, 00, 1d, 02, 0a, 0e, 03, 0e, 02, 0a, 1b, 04, 01, 00, 02]
33
Number of files: 1
44
- file 0 => global file 1
5-
Number of expressions: 6
6-
- expression 0 operands: lhs = Counter(6), rhs = Counter(3)
7-
- expression 1 operands: lhs = Counter(1), rhs = Counter(2)
8-
- expression 2 operands: lhs = Expression(3, Add), rhs = Counter(5)
9-
- expression 3 operands: lhs = Expression(4, Add), rhs = Counter(4)
10-
- expression 4 operands: lhs = Expression(5, Add), rhs = Counter(3)
11-
- expression 5 operands: lhs = Counter(1), rhs = Counter(2)
5+
Number of expressions: 8
6+
- expression 0 operands: lhs = Counter(1), rhs = Counter(3)
7+
- expression 1 operands: lhs = Counter(1), rhs = Expression(5, Add)
8+
- expression 2 operands: lhs = Counter(3), rhs = Counter(4)
9+
- expression 3 operands: lhs = Expression(7, Add), rhs = Expression(5, Add)
10+
- expression 4 operands: lhs = Counter(1), rhs = Counter(2)
11+
- expression 5 operands: lhs = Counter(3), rhs = Counter(4)
12+
- expression 6 operands: lhs = Expression(7, Add), rhs = Counter(5)
13+
- expression 7 operands: lhs = Counter(1), rhs = Counter(2)
1214
Number of file 0 mappings: 13
1315
- Code(Counter(0)) at (prev + 12, 1) to (start + 1, 16)
1416
- Code(Expression(0, Sub)) at (prev + 3, 11) to (start + 0, 12)
15-
= (c6 - c3)
17+
= (c1 - c3)
1618
- Code(Counter(5)) at (prev + 1, 20) to (start + 2, 10)
1719
- Code(Counter(3)) at (prev + 3, 14) to (start + 0, 15)
18-
- Code(Counter(6)) at (prev + 0, 20) to (start + 0, 25)
20+
- Code(Counter(1)) at (prev + 0, 20) to (start + 0, 25)
1921
- Branch { true: Counter(3), false: Expression(0, Sub) } at (prev + 0, 20) to (start + 0, 30)
2022
true = c3
21-
false = (c6 - c3)
23+
false = (c1 - c3)
2224
- Code(Counter(3)) at (prev + 0, 29) to (start + 2, 10)
2325
- Code(Counter(4)) at (prev + 3, 14) to (start + 0, 15)
2426
- Code(Expression(0, Sub)) at (prev + 0, 20) to (start + 0, 25)
25-
= (c6 - c3)
26-
- Branch { true: Counter(4), false: Counter(1) } at (prev + 0, 20) to (start + 0, 30)
27+
= (c1 - c3)
28+
- Branch { true: Counter(4), false: Expression(1, Sub) } at (prev + 0, 20) to (start + 0, 30)
2729
true = c4
28-
false = c1
30+
false = (c1 - (c3 + c4))
2931
- Code(Counter(4)) at (prev + 0, 29) to (start + 2, 10)
30-
- Code(Expression(5, Add)) at (prev + 3, 14) to (start + 2, 10)
31-
= (c1 + c2)
32-
- Code(Expression(2, Add)) at (prev + 4, 1) to (start + 0, 2)
33-
= ((((c1 + c2) + c3) + c4) + c5)
34-
Highest counter ID seen: c6
32+
- Code(Expression(3, Sub)) at (prev + 3, 14) to (start + 2, 10)
33+
= ((c1 + c2) - (c3 + c4))
34+
- Code(Expression(6, Add)) at (prev + 4, 1) to (start + 0, 2)
35+
= ((c1 + c2) + c5)
36+
Highest counter ID seen: c5
3537

0 commit comments

Comments
 (0)