coverage: Remove the loop-based heuristic for choosing expression edges

This heuristic makes sense on paper, but currently isn't worth the extra
complexity, especially when trying to refactor counter creation in other
directions.

Author: Zalathar

compiler/rustc_mir_transform/src/coverage/counters.rs

@@ -299,7 +299,7 @@ impl<'a> MakeBcbCounters<'a> {
         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);
+                self.make_node_counter_and_out_edge_counters(bcb);
             }
         }
 
@@ -312,12 +312,8 @@ impl<'a> MakeBcbCounters<'a> {
 
     /// 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);
@@ -340,8 +336,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;
         };
@@ -455,60 +450,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,

compiler/rustc_mir_transform/src/coverage/graph.rs

@@ -438,16 +438,6 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> {
         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: {:?}",

tests/coverage/abort.cov-map

@@ -1,39 +1,33 @@
 Function name: abort::main
-Raw bytes (89): 0x[01, 01, 0a, 01, 27, 05, 09, 03, 0d, 22, 11, 03, 0d, 03, 0d, 22, 15, 03, 0d, 03, 0d, 05, 09, 0d, 01, 0d, 01, 01, 1b, 03, 02, 0b, 00, 18, 22, 01, 0c, 00, 19, 11, 00, 1a, 02, 0a, 0e, 02, 0a, 00, 0b, 22, 02, 0c, 00, 19, 15, 00, 1a, 00, 31, 1a, 00, 31, 00, 32, 22, 04, 0c, 00, 19, 05, 00, 1a, 00, 31, 09, 00, 31, 00, 32, 27, 01, 09, 00, 17, 0d, 02, 05, 01, 02]
+Raw bytes (81): 0x[01, 01, 06, 01, 13, 05, 09, 0d, 11, 0d, 15, 05, 09, 03, 0d, 0d, 01, 0d, 01, 01, 1b, 03, 02, 0b, 00, 18, 0d, 01, 0c, 00, 19, 11, 00, 1a, 02, 0a, 0a, 02, 0a, 00, 0b, 0d, 02, 0c, 00, 19, 15, 00, 1a, 00, 31, 0e, 00, 31, 00, 32, 0d, 04, 0c, 00, 19, 05, 00, 1a, 00, 31, 09, 00, 31, 00, 32, 13, 01, 09, 00, 17, 16, 02, 05, 01, 02]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 10
-- expression 0 operands: lhs = Counter(0), rhs = Expression(9, Add)
+Number of expressions: 6
+- expression 0 operands: lhs = Counter(0), rhs = Expression(4, Add)
 - expression 1 operands: lhs = Counter(1), rhs = Counter(2)
-- expression 2 operands: lhs = Expression(0, Add), rhs = Counter(3)
-- expression 3 operands: lhs = Expression(8, Sub), rhs = Counter(4)
-- expression 4 operands: lhs = Expression(0, Add), rhs = Counter(3)
+- expression 2 operands: lhs = Counter(3), rhs = Counter(4)
+- expression 3 operands: lhs = Counter(3), rhs = Counter(5)
+- expression 4 operands: lhs = Counter(1), rhs = Counter(2)
 - expression 5 operands: lhs = Expression(0, Add), rhs = Counter(3)
-- expression 6 operands: lhs = Expression(8, Sub), rhs = Counter(5)
-- expression 7 operands: lhs = Expression(0, Add), rhs = Counter(3)
-- expression 8 operands: lhs = Expression(0, Add), rhs = Counter(3)
-- expression 9 operands: lhs = Counter(1), rhs = Counter(2)
 Number of file 0 mappings: 13
 - Code(Counter(0)) at (prev + 13, 1) to (start + 1, 27)
 - Code(Expression(0, Add)) at (prev + 2, 11) to (start + 0, 24)
     = (c0 + (c1 + c2))
-- Code(Expression(8, Sub)) at (prev + 1, 12) to (start + 0, 25)
-    = ((c0 + (c1 + c2)) - c3)
+- Code(Counter(3)) at (prev + 1, 12) to (start + 0, 25)
 - Code(Counter(4)) at (prev + 0, 26) to (start + 2, 10)
-- Code(Expression(3, Sub)) at (prev + 2, 10) to (start + 0, 11)
-    = (((c0 + (c1 + c2)) - c3) - c4)
-- Code(Expression(8, Sub)) at (prev + 2, 12) to (start + 0, 25)
-    = ((c0 + (c1 + c2)) - c3)
+- Code(Expression(2, Sub)) at (prev + 2, 10) to (start + 0, 11)
+    = (c3 - c4)
+- Code(Counter(3)) at (prev + 2, 12) to (start + 0, 25)
 - Code(Counter(5)) at (prev + 0, 26) to (start + 0, 49)
-- Code(Expression(6, Sub)) at (prev + 0, 49) to (start + 0, 50)
-    = (((c0 + (c1 + c2)) - c3) - c5)
-- Code(Expression(8, Sub)) at (prev + 4, 12) to (start + 0, 25)
-    = ((c0 + (c1 + c2)) - c3)
+- Code(Expression(3, Sub)) at (prev + 0, 49) to (start + 0, 50)
+    = (c3 - c5)
+- Code(Counter(3)) at (prev + 4, 12) to (start + 0, 25)
 - Code(Counter(1)) at (prev + 0, 26) to (start + 0, 49)
 - Code(Counter(2)) at (prev + 0, 49) to (start + 0, 50)
-- Code(Expression(9, Add)) at (prev + 1, 9) to (start + 0, 23)
+- Code(Expression(4, Add)) at (prev + 1, 9) to (start + 0, 23)
     = (c1 + c2)
-- Code(Counter(3)) at (prev + 2, 5) to (start + 1, 2)
+- Code(Expression(5, Sub)) at (prev + 2, 5) to (start + 1, 2)
+    = ((c0 + (c1 + c2)) - c3)
 Max counter seen: c5
 
 Function name: abort::might_abort

tests/coverage/assert.cov-map

@@ -1,30 +1,29 @@
 Function name: assert::main
-Raw bytes (65): 0x[01, 01, 08, 01, 1b, 05, 1f, 09, 0d, 03, 11, 16, 05, 03, 11, 05, 1f, 09, 0d, 09, 01, 09, 01, 01, 1b, 03, 02, 0b, 00, 18, 16, 01, 0c, 00, 1a, 05, 00, 1b, 02, 0a, 12, 02, 13, 00, 20, 09, 00, 21, 02, 0a, 0d, 02, 0a, 00, 0b, 1b, 01, 09, 00, 17, 11, 02, 05, 01, 02]
+Raw bytes (63): 0x[01, 01, 07, 01, 13, 05, 17, 09, 0d, 11, 05, 05, 17, 09, 0d, 03, 11, 09, 01, 09, 01, 01, 1b, 03, 02, 0b, 00, 18, 11, 01, 0c, 00, 1a, 05, 00, 1b, 02, 0a, 0e, 02, 13, 00, 20, 09, 00, 21, 02, 0a, 0d, 02, 0a, 00, 0b, 13, 01, 09, 00, 17, 1a, 02, 05, 01, 02]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 8
-- expression 0 operands: lhs = Counter(0), rhs = Expression(6, Add)
-- expression 1 operands: lhs = Counter(1), rhs = Expression(7, Add)
+Number of expressions: 7
+- expression 0 operands: lhs = Counter(0), rhs = Expression(4, Add)
+- expression 1 operands: lhs = Counter(1), rhs = Expression(5, Add)
 - expression 2 operands: lhs = Counter(2), rhs = Counter(3)
-- expression 3 operands: lhs = Expression(0, Add), rhs = Counter(4)
-- expression 4 operands: lhs = Expression(5, Sub), rhs = Counter(1)
-- expression 5 operands: lhs = Expression(0, Add), rhs = Counter(4)
-- expression 6 operands: lhs = Counter(1), rhs = Expression(7, Add)
-- expression 7 operands: lhs = Counter(2), rhs = Counter(3)
+- expression 3 operands: lhs = Counter(4), rhs = Counter(1)
+- expression 4 operands: lhs = Counter(1), rhs = Expression(5, Add)
+- expression 5 operands: lhs = Counter(2), rhs = Counter(3)
+- expression 6 operands: lhs = Expression(0, Add), rhs = Counter(4)
 Number of file 0 mappings: 9
 - Code(Counter(0)) at (prev + 9, 1) to (start + 1, 27)
 - Code(Expression(0, Add)) at (prev + 2, 11) to (start + 0, 24)
     = (c0 + (c1 + (c2 + c3)))
-- Code(Expression(5, Sub)) at (prev + 1, 12) to (start + 0, 26)
-    = ((c0 + (c1 + (c2 + c3))) - c4)
+- Code(Counter(4)) at (prev + 1, 12) to (start + 0, 26)
 - Code(Counter(1)) at (prev + 0, 27) to (start + 2, 10)
-- Code(Expression(4, Sub)) at (prev + 2, 19) to (start + 0, 32)
-    = (((c0 + (c1 + (c2 + c3))) - c4) - c1)
+- Code(Expression(3, Sub)) at (prev + 2, 19) to (start + 0, 32)
+    = (c4 - c1)
 - Code(Counter(2)) at (prev + 0, 33) to (start + 2, 10)
 - Code(Counter(3)) at (prev + 2, 10) to (start + 0, 11)
-- Code(Expression(6, Add)) at (prev + 1, 9) to (start + 0, 23)
+- Code(Expression(4, Add)) at (prev + 1, 9) to (start + 0, 23)
     = (c1 + (c2 + c3))
-- Code(Counter(4)) at (prev + 2, 5) to (start + 1, 2)
+- Code(Expression(6, Sub)) at (prev + 2, 5) to (start + 1, 2)
+    = ((c0 + (c1 + (c2 + c3))) - c4)
 Max counter seen: c4
 
 Function name: assert::might_fail_assert

tests/coverage/assert.coverage

@@ -10,15 +10,15 @@
    LL|      1|fn main() -> Result<(), u8> {
    LL|      1|    let mut countdown = 10;
    LL|     11|    while countdown > 0 {
-   LL|     11|        if countdown == 1 {
+   LL|     10|        if countdown == 1 {
    LL|      1|            might_fail_assert(3);
-   LL|     10|        } else if countdown < 5 {
+   LL|      9|        } else if countdown < 5 {
    LL|      3|            might_fail_assert(2);
    LL|      6|        }
    LL|     10|        countdown -= 1;
    LL|       |    }
-   LL|      0|    Ok(())
-   LL|      0|}
+   LL|      1|    Ok(())
+   LL|      1|}
    LL|       |
    LL|       |// Notes:
    LL|       |//   1. Compare this program and its coverage results to those of the very similar test

tests/coverage/branch/no-mir-spans.cov-map

@@ -23,7 +23,7 @@ Number of file 0 mappings: 2
 Max counter seen: c2
 
 Function name: no_mir_spans::while_op_and
-Raw bytes (25): 0x[01, 01, 01, 09, 0d, 03, 01, 22, 01, 00, 13, 20, 09, 05, 05, 0b, 00, 10, 20, 02, 0d, 00, 14, 00, 19]
+Raw bytes (25): 0x[01, 01, 01, 09, 0d, 03, 01, 22, 01, 00, 13, 20, 09, 05, 05, 0b, 00, 10, 20, 0d, 02, 00, 14, 00, 19]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 1
@@ -33,9 +33,9 @@ Number of file 0 mappings: 3
 - Branch { true: Counter(2), false: Counter(1) } at (prev + 5, 11) to (start + 0, 16)
     true  = c2
     false = c1
-- Branch { true: Expression(0, Sub), false: Counter(3) } at (prev + 0, 20) to (start + 0, 25)
-    true  = (c2 - c3)
-    false = c3
+- Branch { true: Counter(3), false: Expression(0, Sub) } at (prev + 0, 20) to (start + 0, 25)
+    true  = c3
+    false = (c2 - c3)
 Max counter seen: c3
 
 Function name: no_mir_spans::while_op_or

tests/coverage/branch/while.cov-map

@@ -35,30 +35,29 @@ Number of file 0 mappings: 6
 Max counter seen: c2
 
 Function name: while::while_op_and
-Raw bytes (56): 0x[01, 01, 04, 05, 09, 03, 0d, 03, 0d, 11, 0d, 08, 01, 1e, 01, 01, 10, 05, 03, 09, 01, 12, 03, 02, 0b, 00, 10, 20, 0a, 0d, 00, 0b, 00, 10, 0a, 00, 14, 00, 19, 20, 09, 11, 00, 14, 00, 19, 09, 00, 1a, 03, 06, 0f, 04, 01, 00, 02]
+Raw bytes (56): 0x[01, 01, 04, 05, 09, 03, 0d, 11, 0e, 03, 0d, 08, 01, 1e, 01, 01, 10, 05, 03, 09, 01, 12, 03, 02, 0b, 00, 10, 20, 0d, 0e, 00, 0b, 00, 10, 0d, 00, 14, 00, 19, 20, 09, 11, 00, 14, 00, 19, 09, 00, 1a, 03, 06, 0b, 04, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 4
 - expression 0 operands: lhs = Counter(1), rhs = Counter(2)
 - expression 1 operands: lhs = Expression(0, Add), rhs = Counter(3)
-- expression 2 operands: lhs = Expression(0, Add), rhs = Counter(3)
-- expression 3 operands: lhs = Counter(4), rhs = Counter(3)
+- expression 2 operands: lhs = Counter(4), rhs = Expression(3, Sub)
+- expression 3 operands: lhs = Expression(0, Add), rhs = Counter(3)
 Number of file 0 mappings: 8
 - Code(Counter(0)) at (prev + 30, 1) to (start + 1, 16)
 - Code(Counter(1)) at (prev + 3, 9) to (start + 1, 18)
 - Code(Expression(0, Add)) at (prev + 2, 11) to (start + 0, 16)
     = (c1 + c2)
-- Branch { true: Expression(2, Sub), false: Counter(3) } at (prev + 0, 11) to (start + 0, 16)
-    true  = ((c1 + c2) - c3)
-    false = c3
-- Code(Expression(2, Sub)) at (prev + 0, 20) to (start + 0, 25)
-    = ((c1 + c2) - c3)
+- Branch { true: Counter(3), false: Expression(3, Sub) } at (prev + 0, 11) to (start + 0, 16)
+    true  = c3
+    false = ((c1 + c2) - c3)
+- Code(Counter(3)) at (prev + 0, 20) to (start + 0, 25)
 - Branch { true: Counter(2), false: Counter(4) } at (prev + 0, 20) to (start + 0, 25)
     true  = c2
     false = c4
 - Code(Counter(2)) at (prev + 0, 26) to (start + 3, 6)
-- Code(Expression(3, Add)) at (prev + 4, 1) to (start + 0, 2)
-    = (c4 + c3)
+- Code(Expression(2, Add)) at (prev + 4, 1) to (start + 0, 2)
+    = (c4 + ((c1 + c2) - c3))
 Max counter seen: c4
 
 Function name: while::while_op_or