Skip to content

Commit

Permalink
coverage: Simplify how counter terms are stored
Browse files Browse the repository at this point in the history
Using `SmallVec` here was fine when it was a module-private detail, but if we
want to pass these terms across query boundaries then it's not worth the extra
hassle.

Replacing a method call with direct field access is slightly simpler.

Using the name `counter_terms` is more consistent with other code that tries to
maintain a distinction between (physical) "counters" and "expressions".
  • Loading branch information
Zalathar committed Jan 24, 2025
1 parent ff48331 commit 52c1bfa
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 30 deletions.
6 changes: 3 additions & 3 deletions compiler/rustc_mir_transform/src/coverage/counters.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ fn transcribe_counters(
let mut new = CoverageCounters::with_num_bcbs(bcb_needs_counter.domain_size());

for bcb in bcb_needs_counter.iter() {
// Our counter-creation algorithm doesn't guarantee that a counter
// expression starts or ends with a positive term, so partition the
// Our counter-creation algorithm doesn't guarantee that a node's list
// of terms starts or ends with a positive term, so partition the
// counters into "positive" and "negative" lists for easier handling.
let (mut pos, mut neg): (Vec<_>, Vec<_>) =
old.counter_expr(bcb).iter().partition_map(|&CounterTerm { node, op }| match op {
old.counter_terms[bcb].iter().partition_map(|&CounterTerm { node, op }| match op {
Op::Add => Either::Left(node),
Op::Subtract => Either::Right(node),
});
Expand Down
36 changes: 13 additions & 23 deletions compiler/rustc_mir_transform/src/coverage/counters/node_flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use rustc_data_structures::graph;
use rustc_index::bit_set::DenseBitSet;
use rustc_index::{Idx, IndexVec};
use rustc_middle::mir::coverage::Op;
use smallvec::SmallVec;

use crate::coverage::counters::iter_nodes::IterNodes;
use crate::coverage::counters::union_find::{FrozenUnionFind, UnionFind};
Expand Down Expand Up @@ -100,26 +99,20 @@ impl<Node: Idx> MergedNodeFlowGraph<Node> {
builder.visit_node(node);
}

NodeCounters { counter_exprs: builder.finish() }
NodeCounters { counter_terms: builder.finish() }
}
}

/// End result of allocating physical counters and counter expressions for the
/// nodes of a graph.
#[derive(Debug)]
pub(crate) struct NodeCounters<Node: Idx> {
counter_exprs: IndexVec<Node, CounterExprVec<Node>>,
}

impl<Node: Idx> NodeCounters<Node> {
/// For the given node, returns the finished list of terms that represent
/// its physical counter or counter expression. Always non-empty.
///
/// If a node was given a physical counter, its "expression" will contain
/// If a node was given a physical counter, the term list will contain
/// that counter as its sole element.
pub(crate) fn counter_expr(&self, this: Node) -> &[CounterTerm<Node>] {
self.counter_exprs[this].as_slice()
}
pub(crate) counter_terms: IndexVec<Node, Vec<CounterTerm<Node>>>,
}

#[derive(Debug)]
Expand All @@ -146,9 +139,6 @@ pub(crate) struct CounterTerm<Node> {
pub(crate) node: Node,
}

/// Stores the list of counter terms that make up a node's counter expression.
type CounterExprVec<Node> = SmallVec<[CounterTerm<Node>; 2]>;

#[derive(Debug)]
struct SpantreeBuilder<'a, Node: Idx> {
graph: &'a MergedNodeFlowGraph<Node>,
Expand All @@ -163,7 +153,7 @@ struct SpantreeBuilder<'a, Node: Idx> {
yank_buffer: Vec<Node>,
/// An in-progress counter expression for each node. Each expression is
/// initially empty, and will be filled in as relevant nodes are visited.
counter_exprs: IndexVec<Node, CounterExprVec<Node>>,
counter_terms: IndexVec<Node, Vec<CounterTerm<Node>>>,
}

impl<'a, Node: Idx> SpantreeBuilder<'a, Node> {
Expand All @@ -174,7 +164,7 @@ impl<'a, Node: Idx> SpantreeBuilder<'a, Node> {
is_unvisited: DenseBitSet::new_filled(num_nodes),
span_edges: IndexVec::from_fn_n(|_| None, num_nodes),
yank_buffer: vec![],
counter_exprs: IndexVec::from_fn_n(|_| SmallVec::new(), num_nodes),
counter_terms: IndexVec::from_fn_n(|_| vec![], num_nodes),
}
}

Expand Down Expand Up @@ -268,8 +258,8 @@ impl<'a, Node: Idx> SpantreeBuilder<'a, Node> {
// `this_supernode`.

// Instead of setting `this.measure = true` as in the original paper,
// we just add the node's ID to its own "expression".
self.counter_exprs[this].push(CounterTerm { node: this, op: Op::Add });
// we just add the node's ID to its own list of terms.
self.counter_terms[this].push(CounterTerm { node: this, op: Op::Add });

// Walk the spantree from `this.successor` back to `this`. For each
// spantree edge along the way, add this node's physical counter to
Expand All @@ -279,7 +269,7 @@ impl<'a, Node: Idx> SpantreeBuilder<'a, Node> {
let &SpantreeEdge { is_reversed, claiming_node, span_parent } =
self.span_edges[curr].as_ref().unwrap();
let op = if is_reversed { Op::Subtract } else { Op::Add };
self.counter_exprs[claiming_node].push(CounterTerm { node: this, op });
self.counter_terms[claiming_node].push(CounterTerm { node: this, op });

curr = span_parent;
}
Expand All @@ -288,8 +278,8 @@ impl<'a, Node: Idx> SpantreeBuilder<'a, Node> {

/// Asserts that all nodes have been visited, and returns the computed
/// counter expressions (made up of physical counters) for each node.
fn finish(self) -> IndexVec<Node, CounterExprVec<Node>> {
let Self { graph, is_unvisited, span_edges, yank_buffer: _, counter_exprs } = self;
fn finish(self) -> IndexVec<Node, Vec<CounterTerm<Node>>> {
let Self { graph, is_unvisited, span_edges, yank_buffer: _, counter_terms } = self;
assert!(is_unvisited.is_empty(), "some nodes were never visited: {is_unvisited:?}");
debug_assert!(
span_edges
Expand All @@ -298,9 +288,9 @@ impl<'a, Node: Idx> SpantreeBuilder<'a, Node> {
"only supernodes can have a span edge",
);
debug_assert!(
counter_exprs.iter().all(|expr| !expr.is_empty()),
"after visiting all nodes, every node should have a non-empty expression",
counter_terms.iter().all(|terms| !terms.is_empty()),
"after visiting all nodes, every node should have at least one term",
);
counter_exprs
counter_terms
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,12 @@ fn format_counter_expressions<Node: Idx>(counters: &NodeCounters<Node>) -> Vec<S
};

counters
.counter_exprs
.counter_terms
.indices()
.map(|node| {
let mut expr = counters.counter_expr(node).iter().collect::<Vec<_>>();
expr.sort_by_key(|item| item.node.index());
format!("[{node:?}]: {}", expr.into_iter().map(format_item).join(" "))
let mut terms = counters.counter_terms[node].iter().collect::<Vec<_>>();
terms.sort_by_key(|item| item.node.index());
format!("[{node:?}]: {}", terms.into_iter().map(format_item).join(" "))
})
.collect()
}

0 comments on commit 52c1bfa

Please sign in to comment.