coverage: Remove the custom graph traversal from counter creation

The logic behind this traversal order sounds compelling, but in practice it
doesn't seem to produce enough benefit to justify its complexity.

Author: Zalathar

compiler/rustc_mir_transform/src/coverage/counters.rs

@@ -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.
@@ -290,24 +290,9 @@ 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(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

compiler/rustc_mir_transform/src/coverage/graph.rs

@@ -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;
 
@@ -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,
@@ -400,164 +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 }
-    }
-
-    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,

tests/coverage/async.cov-map

@@ -155,25 +155,25 @@ Number of file 0 mappings: 1
 Max counter seen: c0
 
 Function name: async::i::{closure#0}
-Raw bytes (63): 0x[01, 01, 02, 07, 19, 11, 15, 0b, 01, 2d, 13, 04, 0c, 09, 05, 09, 00, 0a, 01, 00, 0e, 00, 18, 05, 00, 1c, 00, 21, 09, 00, 27, 00, 30, 15, 01, 09, 00, 0a, 0d, 00, 0e, 00, 17, 1d, 00, 1b, 00, 20, 15, 00, 24, 00, 26, 19, 01, 0e, 00, 10, 03, 02, 01, 00, 02]
+Raw bytes (63): 0x[01, 01, 02, 07, 15, 0d, 11, 0b, 01, 2d, 13, 04, 0c, 09, 05, 09, 00, 0a, 01, 00, 0e, 00, 18, 05, 00, 1c, 00, 21, 09, 00, 27, 00, 30, 11, 01, 09, 00, 0a, 19, 00, 0e, 00, 17, 1d, 00, 1b, 00, 20, 11, 00, 24, 00, 26, 15, 01, 0e, 00, 10, 03, 02, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
-- expression 0 operands: lhs = Expression(1, Add), rhs = Counter(6)
-- expression 1 operands: lhs = Counter(4), rhs = Counter(5)
+- expression 0 operands: lhs = Expression(1, Add), rhs = Counter(5)
+- expression 1 operands: lhs = Counter(3), rhs = Counter(4)
 Number of file 0 mappings: 11
 - Code(Counter(0)) at (prev + 45, 19) to (start + 4, 12)
 - Code(Counter(2)) at (prev + 5, 9) to (start + 0, 10)
 - Code(Counter(0)) at (prev + 0, 14) to (start + 0, 24)
 - Code(Counter(1)) at (prev + 0, 28) to (start + 0, 33)
 - Code(Counter(2)) at (prev + 0, 39) to (start + 0, 48)
-- Code(Counter(5)) at (prev + 1, 9) to (start + 0, 10)
-- Code(Counter(3)) at (prev + 0, 14) to (start + 0, 23)
+- Code(Counter(4)) at (prev + 1, 9) to (start + 0, 10)
+- Code(Counter(6)) at (prev + 0, 14) to (start + 0, 23)
 - Code(Counter(7)) at (prev + 0, 27) to (start + 0, 32)
-- Code(Counter(5)) at (prev + 0, 36) to (start + 0, 38)
-- Code(Counter(6)) at (prev + 1, 14) to (start + 0, 16)
+- Code(Counter(4)) at (prev + 0, 36) to (start + 0, 38)
+- Code(Counter(5)) at (prev + 1, 14) to (start + 0, 16)
 - Code(Expression(0, Add)) at (prev + 2, 1) to (start + 0, 2)
-    = ((c4 + c5) + c6)
+    = ((c3 + c4) + c5)
 Max counter seen: c7
 
 Function name: async::j

tests/coverage/branch/if.cov-map

@@ -1,28 +1,31 @@
 Function name: if::branch_and
-Raw bytes (56): 0x[01, 01, 04, 05, 09, 0d, 02, 11, 0f, 0d, 02, 08, 01, 2b, 01, 01, 10, 05, 03, 08, 00, 09, 20, 09, 02, 00, 08, 00, 09, 09, 00, 0d, 00, 0e, 20, 11, 0d, 00, 0d, 00, 0e, 11, 00, 0f, 02, 06, 0f, 02, 0c, 02, 06, 0b, 03, 01, 00, 02]
+Raw bytes (62): 0x[01, 01, 07, 05, 09, 09, 0d, 1a, 02, 09, 0d, 0d, 17, 1a, 02, 09, 0d, 08, 01, 2b, 01, 01, 10, 05, 03, 08, 00, 09, 20, 09, 02, 00, 08, 00, 09, 09, 00, 0d, 00, 0e, 20, 0d, 1a, 00, 0d, 00, 0e, 0d, 00, 0f, 02, 06, 17, 02, 0c, 02, 06, 13, 03, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 4
+Number of expressions: 7
 - expression 0 operands: lhs = Counter(1), rhs = Counter(2)
-- expression 1 operands: lhs = Counter(3), rhs = Expression(0, Sub)
-- expression 2 operands: lhs = Counter(4), rhs = Expression(3, Add)
-- expression 3 operands: lhs = Counter(3), rhs = Expression(0, Sub)
+- expression 1 operands: lhs = Counter(2), rhs = Counter(3)
+- expression 2 operands: lhs = Expression(6, Sub), rhs = Expression(0, Sub)
+- expression 3 operands: lhs = Counter(2), rhs = Counter(3)
+- expression 4 operands: lhs = Counter(3), rhs = Expression(5, Add)
+- expression 5 operands: lhs = Expression(6, Sub), rhs = Expression(0, Sub)
+- expression 6 operands: lhs = Counter(2), rhs = Counter(3)
 Number of file 0 mappings: 8
 - Code(Counter(0)) at (prev + 43, 1) to (start + 1, 16)
 - Code(Counter(1)) at (prev + 3, 8) to (start + 0, 9)
 - Branch { true: Counter(2), false: Expression(0, Sub) } at (prev + 0, 8) to (start + 0, 9)
     true  = c2
     false = (c1 - c2)
 - Code(Counter(2)) at (prev + 0, 13) to (start + 0, 14)
-- Branch { true: Counter(4), false: Counter(3) } at (prev + 0, 13) to (start + 0, 14)
-    true  = c4
-    false = c3
-- Code(Counter(4)) at (prev + 0, 15) to (start + 2, 6)
-- Code(Expression(3, Add)) at (prev + 2, 12) to (start + 2, 6)
-    = (c3 + (c1 - c2))
-- Code(Expression(2, Add)) at (prev + 3, 1) to (start + 0, 2)
-    = (c4 + (c3 + (c1 - c2)))
-Max counter seen: c4
+- Branch { true: Counter(3), false: Expression(6, Sub) } at (prev + 0, 13) to (start + 0, 14)
+    true  = c3
+    false = (c2 - c3)
+- Code(Counter(3)) at (prev + 0, 15) to (start + 2, 6)
+- Code(Expression(5, Add)) at (prev + 2, 12) to (start + 2, 6)
+    = ((c2 - c3) + (c1 - c2))
+- Code(Expression(4, Add)) at (prev + 3, 1) to (start + 0, 2)
+    = (c3 + ((c2 - c3) + (c1 - c2)))
+Max counter seen: c3
 
 Function name: if::branch_not
 Raw bytes (116): 0x[01, 01, 07, 05, 09, 05, 0d, 05, 0d, 05, 11, 05, 11, 05, 15, 05, 15, 12, 01, 0c, 01, 01, 10, 05, 03, 08, 00, 09, 20, 09, 02, 00, 08, 00, 09, 09, 01, 09, 00, 11, 02, 01, 06, 00, 07, 05, 01, 08, 00, 0a, 20, 0a, 0d, 00, 08, 00, 0a, 0a, 00, 0b, 02, 06, 0d, 02, 06, 00, 07, 05, 01, 08, 00, 0b, 20, 11, 12, 00, 08, 00, 0b, 11, 00, 0c, 02, 06, 12, 02, 06, 00, 07, 05, 01, 08, 00, 0c, 20, 1a, 15, 00, 08, 00, 0c, 1a, 00, 0d, 02, 06, 15, 02, 06, 00, 07, 05, 01, 01, 00, 02]