mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #116654 - Zalathar:reloop-traversal, r=oli-obk

Browse Source 
coverage: Clarify loop-edge detection and graph traversal

This is a collection of improvements to two semi-related pieces of code:

- The code in `counters` that detects which graph edges don't exit a loop, and would therefore be good candidates to have their coverage computed as an expression rather than having a physical counter.
- The code in `graph` that traverses the coverage BCB graph in a particular order, and tracks loops and loop edges along the way (which is relevant to the above).

I was originally only planning to make the `graph` changes, but there was going to be a lot of indentation churn in `counters` anyway, and once I started looking I noticed a lot of opportunities for simplification.

---

`@rustbot` label +A-code-coverage
This commit is contained in:
Matthias Krüger 2023-10-12 18:36:44 +02:00 committed by GitHub
commit 4832811b0d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 128 additions and 162 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

129
compiler/rustc_mir_transform/src/coverage/counters.rs
View File

@ -245,13 +245,13 @@ fn make_bcb_counters(
// the loop. The `traversal` state includes a `context_stack`, providing a way to know if // the loop. The `traversal` state includes a `context_stack`, providing a way to know if
// the current BCB is in one or more nested loops or not. // the current BCB is in one or more nested loops or not.
let mut traversal = TraverseCoverageGraphWithLoops::new(&self.basic_coverage_blocks); let mut traversal = TraverseCoverageGraphWithLoops::new(&self.basic_coverage_blocks);
while let Some(bcb) = traversal.next(self.basic_coverage_blocks) { while let Some(bcb) = traversal.next() {
if bcb_has_coverage_spans(bcb) { if bcb_has_coverage_spans(bcb) {
debug!("{:?} has at least one coverage span. Get or make its counter", bcb); debug!("{:?} has at least one coverage span. Get or make its counter", bcb);
let branching_counter_operand = self.get_or_make_counter_operand(bcb)?; let branching_counter_operand = self.get_or_make_counter_operand(bcb)?;
if self.bcb_needs_branch_counters(bcb) { if self.bcb_needs_branch_counters(bcb) {
self.make_branch_counters(&mut traversal, bcb, branching_counter_operand)?; self.make_branch_counters(&traversal, bcb, branching_counter_operand)?;
} }
} else { } else {
debug!( debug!(
@ -274,7 +274,7 @@ fn make_bcb_counters(
fn make_branch_counters( fn make_branch_counters(
&mut self, &mut self,
traversal: &mut TraverseCoverageGraphWithLoops, traversal: &TraverseCoverageGraphWithLoops<'_>,
branching_bcb: BasicCoverageBlock, branching_bcb: BasicCoverageBlock,
branching_counter_operand: Operand, branching_counter_operand: Operand,
) -> Result<(), Error> { ) -> Result<(), Error> {
@ -507,21 +507,14 @@ fn recursive_get_or_make_edge_counter_operand(
/// found, select any branch. /// found, select any branch.
fn choose_preferred_expression_branch( fn choose_preferred_expression_branch(
&self, &self,
traversal: &TraverseCoverageGraphWithLoops, traversal: &TraverseCoverageGraphWithLoops<'_>,
branches: &[BcbBranch], branches: &[BcbBranch],
) -> BcbBranch { ) -> BcbBranch {
let branch_needs_a_counter = |branch: &BcbBranch| self.branch_has_no_counter(branch); let good_reloop_branch = self.find_good_reloop_branch(traversal, &branches);
if let Some(reloop_branch) = good_reloop_branch {
let some_reloop_branch = self.find_some_reloop_branch(traversal, &branches); assert!(self.branch_has_no_counter(&reloop_branch));
if let Some(reloop_branch_without_counter) = debug!("Selecting reloop branch {reloop_branch:?} to get an expression");
some_reloop_branch.filter(branch_needs_a_counter) reloop_branch
{
debug!(
"Selecting reloop_branch={:?} that still needs a counter, to get the \
`Expression`",
reloop_branch_without_counter
);
reloop_branch_without_counter
} else { } else {
let &branch_without_counter = let &branch_without_counter =
branches.iter().find(|&branch| self.branch_has_no_counter(branch)).expect( branches.iter().find(|&branch| self.branch_has_no_counter(branch)).expect(
@ -538,75 +531,52 @@ fn choose_preferred_expression_branch(
} }
} }
/// At most, one of the branches (or its edge, from the branching_bcb, if the branch has /// Tries to find a branch that leads back to the top of a loop, and that
/// multiple incoming edges) can have a counter computed by expression. /// doesn't already have a counter. Such branches are good candidates to
/// /// be given an expression (instead of a physical counter), because they
/// If at least one of the branches leads outside of a loop (`found_loop_exit` is /// will tend to be executed more times than a loop-exit branch.
/// true), and at least one other branch does not exit the loop (the first of which fn find_good_reloop_branch(
/// is captured in `some_reloop_branch`), it's likely any reloop branch will be
/// executed far more often than loop exit branch, making the reloop branch a better
/// candidate for an expression.
fn find_some_reloop_branch(
&self, &self,
traversal: &TraverseCoverageGraphWithLoops, traversal: &TraverseCoverageGraphWithLoops<'_>,
branches: &[BcbBranch], branches: &[BcbBranch],
) -> Option<BcbBranch> { ) -> Option<BcbBranch> {
let branch_needs_a_counter = |branch: &BcbBranch| self.branch_has_no_counter(branch); // Consider each loop on the current traversal context stack, top-down.
for reloop_bcbs in traversal.reloop_bcbs_per_loop() {
let mut all_branches_exit_this_loop = true;
let mut some_reloop_branch: Option<BcbBranch> = None; // Try to find a branch that doesn't exit this loop and doesn't
for context in traversal.context_stack.iter().rev() { // already have a counter.
if let Some((backedge_from_bcbs, _)) = &context.loop_backedges { for &branch in branches {
let mut found_loop_exit = false; // A branch is a reloop branch if it dominates any BCB that has
for &branch in branches.iter() { // an edge back to the loop header. (Other branches are exits.)
if backedge_from_bcbs.iter().any(|&backedge_from_bcb| { let is_reloop_branch = reloop_bcbs.iter().any(|&reloop_bcb| {
self.bcb_dominates(branch.target_bcb, backedge_from_bcb) self.basic_coverage_blocks.dominates(branch.target_bcb, reloop_bcb)
}) { });
if let Some(reloop_branch) = some_reloop_branch {
if self.branch_has_no_counter(&reloop_branch) { if is_reloop_branch {
// we already found a candidate reloop_branch that still all_branches_exit_this_loop = false;
// needs a counter if self.branch_has_no_counter(&branch) {
continue; // We found a good branch to be given an expression.
} return Some(branch);
}
// The path from branch leads back to the top of the loop. Set this
// branch as the `reloop_branch`. If this branch already has a
// counter, and we find another reloop branch that doesn't have a
// counter yet, that branch will be selected as the `reloop_branch`
// instead.
some_reloop_branch = Some(branch);
} else {
// The path from branch leads outside this loop
found_loop_exit = true;
}
if found_loop_exit
&& some_reloop_branch.filter(branch_needs_a_counter).is_some()
{
// Found both a branch that exits the loop and a branch that returns
// to the top of the loop (`reloop_branch`), and the `reloop_branch`
// doesn't already have a counter.
break;
} }
// Keep looking for another reloop branch without a counter.
} else {
// This branch exits the loop.
} }
if !found_loop_exit {
debug!(
"No branches exit the loop, so any branch without an existing \
counter can have the `Expression`."
);
break;
}
if some_reloop_branch.is_some() {
debug!(
"Found a branch that exits the loop and a branch the loops back to \
the top of the loop (`reloop_branch`). The `reloop_branch` will \
get the `Expression`, as long as it still needs a counter."
);
break;
}
// else all branches exited this loop context, so run the same checks with
// the outer loop(s)
} }
if !all_branches_exit_this_loop {
// We found one or more reloop branches, but all of them already
// have counters. Let the caller choose one of the exit branches.
debug!("All reloop branches had counters; skip checking the other loops");
return None;
}
// All of the branches exit this loop, so keep looking for a good
// reloop branch for one of the outer loops.
} }
some_reloop_branch
None
} }
#[inline] #[inline]
@ -652,9 +622,4 @@ fn branch_counter(&self, branch: &BcbBranch) -> Option<&BcbCounter> {
fn bcb_has_one_path_to_target(&self, bcb: BasicCoverageBlock) -> bool { fn bcb_has_one_path_to_target(&self, bcb: BasicCoverageBlock) -> bool {
self.bcb_predecessors(bcb).len() <= 1 self.bcb_predecessors(bcb).len() <= 1
} }
#[inline]
fn bcb_dominates(&self, dom: BasicCoverageBlock, node: BasicCoverageBlock) -> bool {
self.basic_coverage_blocks.dominates(dom, node)
}
} }

159
compiler/rustc_mir_transform/src/coverage/graph.rs
View File

@ -6,6 +6,7 @@
use rustc_middle::mir::{self, BasicBlock, TerminatorKind}; use rustc_middle::mir::{self, BasicBlock, TerminatorKind};
use std::cmp::Ordering; use std::cmp::Ordering;
use std::collections::VecDeque;
use std::ops::{Index, IndexMut}; use std::ops::{Index, IndexMut};
/// A coverage-specific simplification of the MIR control flow graph (CFG). The `CoverageGraph`s /// A coverage-specific simplification of the MIR control flow graph (CFG). The `CoverageGraph`s
@ -385,57 +386,72 @@ fn bcb_filtered_successors<'a, 'tcx>(
/// ensures a loop is completely traversed before processing Blocks after the end of the loop. /// ensures a loop is completely traversed before processing Blocks after the end of the loop.
#[derive(Debug)] #[derive(Debug)]
pub(super) struct TraversalContext { pub(super) struct TraversalContext {
/// From one or more backedges returning to a loop header. /// BCB with one or more incoming loop backedges, indicating which loop
pub loop_backedges: Option<(Vec<BasicCoverageBlock>, BasicCoverageBlock)>, /// this context is for.
///
/// If `None`, this is the non-loop context for the function as a whole.
loop_header: Option<BasicCoverageBlock>,
/// worklist, to be traversed, of CoverageGraph in the loop with the given loop /// Worklist of BCBs to be processed in this context.
/// backedges, such that the loop is the inner inner-most loop containing these worklist: VecDeque<BasicCoverageBlock>,
/// CoverageGraph
pub worklist: Vec<BasicCoverageBlock>,
} }
pub(super) struct TraverseCoverageGraphWithLoops { pub(super) struct TraverseCoverageGraphWithLoops<'a> {
pub backedges: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>, basic_coverage_blocks: &'a CoverageGraph,
pub context_stack: Vec<TraversalContext>,
backedges: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>,
context_stack: Vec<TraversalContext>,
visited: BitSet<BasicCoverageBlock>, visited: BitSet<BasicCoverageBlock>,
} }
impl TraverseCoverageGraphWithLoops { impl<'a> TraverseCoverageGraphWithLoops<'a> {
pub fn new(basic_coverage_blocks: &CoverageGraph) -> Self { pub(super) fn new(basic_coverage_blocks: &'a CoverageGraph) -> Self {
let start_bcb = basic_coverage_blocks.start_node();
let backedges = find_loop_backedges(basic_coverage_blocks); let backedges = find_loop_backedges(basic_coverage_blocks);
let context_stack =
vec![TraversalContext { loop_backedges: None, worklist: vec![start_bcb] }]; 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 // `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 // 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 // of the stack as loops are entered, and popped off of the stack when a loop's worklist is
// exhausted. // exhausted.
let visited = BitSet::new_empty(basic_coverage_blocks.num_nodes()); let visited = BitSet::new_empty(basic_coverage_blocks.num_nodes());
Self { backedges, context_stack, visited } Self { basic_coverage_blocks, backedges, context_stack, visited }
} }
pub fn next(&mut self, basic_coverage_blocks: &CoverageGraph) -> Option<BasicCoverageBlock> { /// 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(super) 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(super) fn next(&mut self) -> Option<BasicCoverageBlock> {
debug!( debug!(
"TraverseCoverageGraphWithLoops::next - context_stack: {:?}", "TraverseCoverageGraphWithLoops::next - context_stack: {:?}",
self.context_stack.iter().rev().collect::<Vec<_>>() self.context_stack.iter().rev().collect::<Vec<_>>()
); );
while let Some(context) = self.context_stack.last_mut() { while let Some(context) = self.context_stack.last_mut() {
if let Some(next_bcb) = context.worklist.pop() { if let Some(bcb) = context.worklist.pop_front() {
if !self.visited.insert(next_bcb) { if !self.visited.insert(bcb) {
debug!("Already visited: {:?}", next_bcb); debug!("Already visited: {bcb:?}");
continue; continue;
} }
debug!("Visiting {:?}", next_bcb); debug!("Visiting {bcb:?}");
if self.backedges[next_bcb].len() > 0 {
debug!("{:?} is a loop header! Start a new TraversalContext...", next_bcb); if self.backedges[bcb].len() > 0 {
debug!("{bcb:?} is a loop header! Start a new TraversalContext...");
self.context_stack.push(TraversalContext { self.context_stack.push(TraversalContext {
loop_backedges: Some((self.backedges[next_bcb].clone(), next_bcb)), loop_header: Some(bcb),
worklist: Vec::new(), worklist: VecDeque::new(),
}); });
} }
self.extend_worklist(basic_coverage_blocks, next_bcb); self.add_successors_to_worklists(bcb);
return Some(next_bcb); return Some(bcb);
} else { } else {
// Strip contexts with empty worklists from the top of the stack // Strip contexts with empty worklists from the top of the stack
self.context_stack.pop(); self.context_stack.pop();
@ -445,13 +461,10 @@ pub fn next(&mut self, basic_coverage_blocks: &CoverageGraph) -> Option<BasicCov
None None
} }
pub fn extend_worklist( pub fn add_successors_to_worklists(&mut self, bcb: BasicCoverageBlock) {
&mut self, let successors = &self.basic_coverage_blocks.successors[bcb];
basic_coverage_blocks: &CoverageGraph,
bcb: BasicCoverageBlock,
) {
let successors = &basic_coverage_blocks.successors[bcb];
debug!("{:?} has {} successors:", bcb, successors.len()); debug!("{:?} has {} successors:", bcb, successors.len());
for &successor in successors { for &successor in successors {
if successor == bcb { if successor == bcb {
debug!( debug!(
@ -460,56 +473,44 @@ pub fn extend_worklist(
bcb bcb
); );
// Don't re-add this successor to the worklist. We are already processing it. // 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; break;
} }
for context in self.context_stack.iter_mut().rev() {
// Add successors of the current BCB to the appropriate context. Successors that // 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 // 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 // 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 // from other BCBs outside the loop, and they will be added to a different
// worklist. // worklist.
// //
// Branching blocks (with more than one successor) must be processed before // Branching blocks (with more than one successor) must be processed before
// blocks with only one successor, to prevent unnecessarily complicating // blocks with only one successor, to prevent unnecessarily complicating
// `Expression`s by creating a Counter in a `BasicCoverageBlock` that the // `Expression`s by creating a Counter in a `BasicCoverageBlock` that the
// branching block would have given an `Expression` (or vice versa). // branching block would have given an `Expression` (or vice versa).
let (some_successor_to_add, some_loop_header) =
if let Some((_, loop_header)) = context.loop_backedges { let context = self
if basic_coverage_blocks.dominates(loop_header, successor) { .context_stack
(Some(successor), Some(loop_header)) .iter_mut()
} else { .rev()
(None, None) .find(|context| match context.loop_header {
} Some(loop_header) => {
} else { self.basic_coverage_blocks.dominates(loop_header, successor)
(Some(successor), None)
};
if let Some(successor_to_add) = some_successor_to_add {
if basic_coverage_blocks.successors[successor_to_add].len() > 1 {
debug!(
"{:?} successor is branching. Prioritize it at the beginning of \
the {}",
successor_to_add,
if let Some(loop_header) = some_loop_header {
format!("worklist for the loop headed by {loop_header:?}")
} else {
String::from("non-loop worklist")
},
);
context.worklist.insert(0, successor_to_add);
} else {
debug!(
"{:?} successor is non-branching. Defer it to the end of the {}",
successor_to_add,
if let Some(loop_header) = some_loop_header {
format!("worklist for the loop headed by {loop_header:?}")
} else {
String::from("non-loop worklist")
},
);
context.worklist.push(successor_to_add);
} }
break; 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);
} }
} }
} }

2
compiler/rustc_mir_transform/src/coverage/tests.rs
View File

@ -628,7 +628,7 @@ fn test_traverse_coverage_with_loops() {
let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body); let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
let mut traversed_in_order = Vec::new(); let mut traversed_in_order = Vec::new();
let mut traversal = graph::TraverseCoverageGraphWithLoops::new(&basic_coverage_blocks); let mut traversal = graph::TraverseCoverageGraphWithLoops::new(&basic_coverage_blocks);
while let Some(bcb) = traversal.next(&basic_coverage_blocks) { while let Some(bcb) = traversal.next() {
traversed_in_order.push(bcb); traversed_in_order.push(bcb);
} }