From a7ae2a6e6c68f0af5c3afcac015eea2bd18f6e27 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 12 Oct 2023 17:41:11 +1100 Subject: [PATCH 1/6] coverage: Simplify the detection of reloop edges to be given expressions --- .../src/coverage/counters.rs | 121 +++++++----------- .../rustc_mir_transform/src/coverage/graph.rs | 18 ++- 2 files changed, 57 insertions(+), 82 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 78845af016276..06770dafbaa06 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -510,18 +510,11 @@ impl<'a> MakeBcbCounters<'a> { traversal: &TraverseCoverageGraphWithLoops, branches: &[BcbBranch], ) -> BcbBranch { - let branch_needs_a_counter = |branch: &BcbBranch| self.branch_has_no_counter(branch); - - let some_reloop_branch = self.find_some_reloop_branch(traversal, &branches); - if let Some(reloop_branch_without_counter) = - some_reloop_branch.filter(branch_needs_a_counter) - { - debug!( - "Selecting reloop_branch={:?} that still needs a counter, to get the \ - `Expression`", - reloop_branch_without_counter - ); - reloop_branch_without_counter + let good_reloop_branch = self.find_good_reloop_branch(traversal, &branches); + if let Some(reloop_branch) = good_reloop_branch { + assert!(self.branch_has_no_counter(&reloop_branch)); + debug!("Selecting reloop branch {reloop_branch:?} to get an expression"); + reloop_branch } else { let &branch_without_counter = branches.iter().find(|&branch| self.branch_has_no_counter(branch)).expect( @@ -538,75 +531,52 @@ impl<'a> MakeBcbCounters<'a> { } } - /// At most, one of the branches (or its edge, from the branching_bcb, if the branch has - /// multiple incoming edges) can have a counter computed by expression. - /// - /// If at least one of the branches leads outside of a loop (`found_loop_exit` is - /// true), and at least one other branch does not exit the loop (the first of which - /// 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( + /// Tries to find a branch that leads back to the top of a loop, and that + /// doesn't already have a counter. Such branches are good candidates to + /// be given an expression (instead of a physical counter), because they + /// will tend to be executed more times than a loop-exit branch. + fn find_good_reloop_branch( &self, traversal: &TraverseCoverageGraphWithLoops, branches: &[BcbBranch], ) -> Option { - let branch_needs_a_counter = |branch: &BcbBranch| self.branch_has_no_counter(branch); - - let mut some_reloop_branch: Option = None; - for context in traversal.context_stack.iter().rev() { - if let Some((backedge_from_bcbs, _)) = &context.loop_backedges { - let mut found_loop_exit = false; - for &branch in branches.iter() { - if backedge_from_bcbs.iter().any(|&backedge_from_bcb| { - self.bcb_dominates(branch.target_bcb, backedge_from_bcb) - }) { - if let Some(reloop_branch) = some_reloop_branch { - if self.branch_has_no_counter(&reloop_branch) { - // we already found a candidate reloop_branch that still - // needs a counter - continue; - } - } - // 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; + // 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; + + // Try to find a branch that doesn't exit this loop and doesn't + // already have a counter. + for &branch in branches { + // A branch is a reloop branch if it dominates any BCB that has + // an edge back to the loop header. (Other branches are exits.) + let is_reloop_branch = reloop_bcbs.iter().any(|&reloop_bcb| { + self.basic_coverage_blocks.dominates(branch.target_bcb, reloop_bcb) + }); + + if is_reloop_branch { + all_branches_exit_this_loop = false; + if self.branch_has_no_counter(&branch) { + // We found a good branch to be given an expression. + return Some(branch); } + // 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] @@ -652,9 +622,4 @@ impl<'a> MakeBcbCounters<'a> { fn bcb_has_one_path_to_target(&self, bcb: BasicCoverageBlock) -> bool { self.bcb_predecessors(bcb).len() <= 1 } - - #[inline] - fn bcb_dominates(&self, dom: BasicCoverageBlock, node: BasicCoverageBlock) -> bool { - self.basic_coverage_blocks.dominates(dom, node) - } } diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index 39027d21f066a..cf917ecee8664 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -386,17 +386,17 @@ fn bcb_filtered_successors<'a, 'tcx>( #[derive(Debug)] pub(super) struct TraversalContext { /// From one or more backedges returning to a loop header. - pub loop_backedges: Option<(Vec, BasicCoverageBlock)>, + loop_backedges: Option<(Vec, BasicCoverageBlock)>, /// worklist, to be traversed, of CoverageGraph in the loop with the given loop /// backedges, such that the loop is the inner inner-most loop containing these /// CoverageGraph - pub worklist: Vec, + worklist: Vec, } pub(super) struct TraverseCoverageGraphWithLoops { - pub backedges: IndexVec>, - pub context_stack: Vec, + backedges: IndexVec>, + context_stack: Vec, visited: BitSet, } @@ -414,6 +414,16 @@ impl TraverseCoverageGraphWithLoops { Self { 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(super) fn reloop_bcbs_per_loop(&self) -> impl Iterator { + self.context_stack + .iter() + .rev() + .filter_map(|context| context.loop_backedges.as_ref()) + .map(|(from_bcbs, _to_bcb)| from_bcbs.as_slice()) + } + pub fn next(&mut self, basic_coverage_blocks: &CoverageGraph) -> Option { debug!( "TraverseCoverageGraphWithLoops::next - context_stack: {:?}", From d1920c518151b29e3326cdacc757a219817f3c73 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 12 Oct 2023 12:59:11 +1100 Subject: [PATCH 2/6] coverage: Rename `next_bcb` to just `bcb` This is the only BCB that `TraverseCoverageGraphWithLoops::next` works with, so calling it `next_bcb` just makes the code less clear. --- .../rustc_mir_transform/src/coverage/graph.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index cf917ecee8664..12fb897bd79c3 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -431,21 +431,22 @@ impl TraverseCoverageGraphWithLoops { ); while let Some(context) = self.context_stack.last_mut() { - if let Some(next_bcb) = context.worklist.pop() { - if !self.visited.insert(next_bcb) { - debug!("Already visited: {:?}", next_bcb); + if let Some(bcb) = context.worklist.pop() { + if !self.visited.insert(bcb) { + debug!("Already visited: {bcb:?}"); continue; } - debug!("Visiting {:?}", next_bcb); - if self.backedges[next_bcb].len() > 0 { - debug!("{:?} is a loop header! Start a new TraversalContext...", next_bcb); + 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_backedges: Some((self.backedges[next_bcb].clone(), next_bcb)), + loop_backedges: Some((self.backedges[bcb].clone(), bcb)), worklist: Vec::new(), }); } - self.extend_worklist(basic_coverage_blocks, next_bcb); - return Some(next_bcb); + self.extend_worklist(basic_coverage_blocks, bcb); + return Some(bcb); } else { // Strip contexts with empty worklists from the top of the stack self.context_stack.pop(); From ea3fb7bc2cd64fc124209e6f4ffcda8d053fd7ae Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 12 Oct 2023 12:32:21 +1100 Subject: [PATCH 3/6] coverage: Use a `VecDeque` for loop traversal worklists The previous code was storing the worklist in a vector, and then selectively adding items to the start or end of the vector. That's a perfect use-case for a double-ended queue. This change also reveals that the existing code was a bit confused about which end of the worklist is the front or back. For now, items are always removed from the front of the queue (instead of the back), and code that adds items to the queue has been flipped, to preserve the existing behaviour. --- .../rustc_mir_transform/src/coverage/graph.rs | 49 +++++++------------ 1 file changed, 18 insertions(+), 31 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index 12fb897bd79c3..8dc13117b9f4c 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -6,6 +6,7 @@ use rustc_index::{IndexSlice, IndexVec}; use rustc_middle::mir::{self, BasicBlock, TerminatorKind}; use std::cmp::Ordering; +use std::collections::VecDeque; use std::ops::{Index, IndexMut}; /// A coverage-specific simplification of the MIR control flow graph (CFG). The `CoverageGraph`s @@ -388,10 +389,8 @@ pub(super) struct TraversalContext { /// From one or more backedges returning to a loop header. loop_backedges: Option<(Vec, BasicCoverageBlock)>, - /// worklist, to be traversed, of CoverageGraph in the loop with the given loop - /// backedges, such that the loop is the inner inner-most loop containing these - /// CoverageGraph - worklist: Vec, + /// Worklist of BCBs to be processed in this context. + worklist: VecDeque, } pub(super) struct TraverseCoverageGraphWithLoops { @@ -402,10 +401,11 @@ pub(super) struct TraverseCoverageGraphWithLoops { impl TraverseCoverageGraphWithLoops { pub fn new(basic_coverage_blocks: &CoverageGraph) -> Self { - let start_bcb = basic_coverage_blocks.start_node(); 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_backedges: 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 @@ -431,7 +431,7 @@ impl TraverseCoverageGraphWithLoops { ); while let Some(context) = self.context_stack.last_mut() { - if let Some(bcb) = context.worklist.pop() { + if let Some(bcb) = context.worklist.pop_front() { if !self.visited.insert(bcb) { debug!("Already visited: {bcb:?}"); continue; @@ -442,7 +442,7 @@ impl TraverseCoverageGraphWithLoops { debug!("{bcb:?} is a loop header! Start a new TraversalContext..."); self.context_stack.push(TraversalContext { loop_backedges: Some((self.backedges[bcb].clone(), bcb)), - worklist: Vec::new(), + worklist: VecDeque::new(), }); } self.extend_worklist(basic_coverage_blocks, bcb); @@ -484,7 +484,7 @@ impl TraverseCoverageGraphWithLoops { // 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 (some_successor_to_add, some_loop_header) = + let (some_successor_to_add, _) = if let Some((_, loop_header)) = context.loop_backedges { if basic_coverage_blocks.dominates(loop_header, successor) { (Some(successor), Some(loop_header)) @@ -494,30 +494,17 @@ impl TraverseCoverageGraphWithLoops { } else { (Some(successor), None) }; + + // 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 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); + context.worklist.push_back(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); + context.worklist.push_front(successor_to_add); } break; } From 15360b3bc8f75c33af8a41da5dc9a1cab19691ba Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 11 Oct 2023 17:40:37 +1100 Subject: [PATCH 4/6] coverage: Store a graph reference in the graph traversal struct Having to keep passing in a graph reference was a holdover from when the graph was partly mutated during traversal. As of #114354 that is no longer necessary, so we can simplify the traversal code by storing a graph reference as a field in `TraverseCoverageGraphWithLoops`. --- .../src/coverage/counters.rs | 10 ++++----- .../rustc_mir_transform/src/coverage/graph.rs | 21 +++++++++---------- .../rustc_mir_transform/src/coverage/tests.rs | 2 +- 3 files changed, 16 insertions(+), 17 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs index 06770dafbaa06..77e3dee1fefa1 100644 --- a/compiler/rustc_mir_transform/src/coverage/counters.rs +++ b/compiler/rustc_mir_transform/src/coverage/counters.rs @@ -245,13 +245,13 @@ impl<'a> MakeBcbCounters<'a> { // 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. 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) { debug!("{:?} has at least one coverage span. Get or make its counter", bcb); let branching_counter_operand = self.get_or_make_counter_operand(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 { debug!( @@ -274,7 +274,7 @@ impl<'a> MakeBcbCounters<'a> { fn make_branch_counters( &mut self, - traversal: &mut TraverseCoverageGraphWithLoops, + traversal: &TraverseCoverageGraphWithLoops<'_>, branching_bcb: BasicCoverageBlock, branching_counter_operand: Operand, ) -> Result<(), Error> { @@ -507,7 +507,7 @@ impl<'a> MakeBcbCounters<'a> { /// found, select any branch. fn choose_preferred_expression_branch( &self, - traversal: &TraverseCoverageGraphWithLoops, + traversal: &TraverseCoverageGraphWithLoops<'_>, branches: &[BcbBranch], ) -> BcbBranch { let good_reloop_branch = self.find_good_reloop_branch(traversal, &branches); @@ -537,7 +537,7 @@ impl<'a> MakeBcbCounters<'a> { /// will tend to be executed more times than a loop-exit branch. fn find_good_reloop_branch( &self, - traversal: &TraverseCoverageGraphWithLoops, + traversal: &TraverseCoverageGraphWithLoops<'_>, branches: &[BcbBranch], ) -> Option { // Consider each loop on the current traversal context stack, top-down. diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index 8dc13117b9f4c..02bc62a7d2660 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -393,14 +393,16 @@ pub(super) struct TraversalContext { worklist: VecDeque, } -pub(super) struct TraverseCoverageGraphWithLoops { +pub(super) struct TraverseCoverageGraphWithLoops<'a> { + basic_coverage_blocks: &'a CoverageGraph, + backedges: IndexVec>, context_stack: Vec, visited: BitSet, } -impl TraverseCoverageGraphWithLoops { - pub fn new(basic_coverage_blocks: &CoverageGraph) -> Self { +impl<'a> TraverseCoverageGraphWithLoops<'a> { + pub(super) 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()]); @@ -411,7 +413,7 @@ impl TraverseCoverageGraphWithLoops { // 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 { backedges, context_stack, visited } + Self { basic_coverage_blocks, backedges, context_stack, visited } } /// For each loop on the loop context stack (top-down), yields a list of BCBs @@ -424,7 +426,7 @@ impl TraverseCoverageGraphWithLoops { .map(|(from_bcbs, _to_bcb)| from_bcbs.as_slice()) } - pub fn next(&mut self, basic_coverage_blocks: &CoverageGraph) -> Option { + pub(super) fn next(&mut self) -> Option { debug!( "TraverseCoverageGraphWithLoops::next - context_stack: {:?}", self.context_stack.iter().rev().collect::>() @@ -445,7 +447,7 @@ impl TraverseCoverageGraphWithLoops { worklist: VecDeque::new(), }); } - self.extend_worklist(basic_coverage_blocks, bcb); + self.extend_worklist(bcb); return Some(bcb); } else { // Strip contexts with empty worklists from the top of the stack @@ -456,11 +458,8 @@ impl TraverseCoverageGraphWithLoops { None } - pub fn extend_worklist( - &mut self, - basic_coverage_blocks: &CoverageGraph, - bcb: BasicCoverageBlock, - ) { + pub fn extend_worklist(&mut self, bcb: BasicCoverageBlock) { + let Self { basic_coverage_blocks, .. } = *self; let successors = &basic_coverage_blocks.successors[bcb]; debug!("{:?} has {} successors:", bcb, successors.len()); for &successor in successors { diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs index ee7cb791c8180..487d22823644b 100644 --- a/compiler/rustc_mir_transform/src/coverage/tests.rs +++ b/compiler/rustc_mir_transform/src/coverage/tests.rs @@ -628,7 +628,7 @@ fn test_traverse_coverage_with_loops() { let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body); let mut traversed_in_order = Vec::new(); 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); } From 59f4f1c89d035bffbb6455f1dc25a45f09c837a5 Mon Sep 17 00:00:00 2001 From: Zalathar Date: Wed, 11 Oct 2023 20:33:26 +1100 Subject: [PATCH 5/6] coverage: Don't store loop backedges in the traversal context As long as we store the loop header BCB, we can look up its incoming loop backedges as needed. --- .../rustc_mir_transform/src/coverage/graph.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index 02bc62a7d2660..2a88a5f7f44e2 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -386,8 +386,11 @@ fn bcb_filtered_successors<'a, 'tcx>( /// ensures a loop is completely traversed before processing Blocks after the end of the loop. #[derive(Debug)] pub(super) struct TraversalContext { - /// From one or more backedges returning to a loop header. - loop_backedges: Option<(Vec, BasicCoverageBlock)>, + /// 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, /// Worklist of BCBs to be processed in this context. worklist: VecDeque, @@ -406,7 +409,7 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> { let backedges = find_loop_backedges(basic_coverage_blocks); let worklist = VecDeque::from([basic_coverage_blocks.start_node()]); - let context_stack = vec![TraversalContext { loop_backedges: None, worklist }]; + 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 @@ -422,8 +425,8 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> { self.context_stack .iter() .rev() - .filter_map(|context| context.loop_backedges.as_ref()) - .map(|(from_bcbs, _to_bcb)| from_bcbs.as_slice()) + .filter_map(|context| context.loop_header) + .map(|header_bcb| self.backedges[header_bcb].as_slice()) } pub(super) fn next(&mut self) -> Option { @@ -443,7 +446,7 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> { if self.backedges[bcb].len() > 0 { debug!("{bcb:?} is a loop header! Start a new TraversalContext..."); self.context_stack.push(TraversalContext { - loop_backedges: Some((self.backedges[bcb].clone(), bcb)), + loop_header: Some(bcb), worklist: VecDeque::new(), }); } @@ -484,7 +487,7 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> { // `Expression`s by creating a Counter in a `BasicCoverageBlock` that the // branching block would have given an `Expression` (or vice versa). let (some_successor_to_add, _) = - if let Some((_, loop_header)) = context.loop_backedges { + if let Some(loop_header) = context.loop_header { if basic_coverage_blocks.dominates(loop_header, successor) { (Some(successor), Some(loop_header)) } else { From d99ab97b027917b434790b76492386310b47978d Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 12 Oct 2023 13:31:35 +1100 Subject: [PATCH 6/6] coverage: Simplify adding BCB successors to the traversal worklists --- .../rustc_mir_transform/src/coverage/graph.rs | 77 ++++++++++--------- 1 file changed, 39 insertions(+), 38 deletions(-) diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs index 2a88a5f7f44e2..9a7adaada09fc 100644 --- a/compiler/rustc_mir_transform/src/coverage/graph.rs +++ b/compiler/rustc_mir_transform/src/coverage/graph.rs @@ -450,7 +450,7 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> { worklist: VecDeque::new(), }); } - self.extend_worklist(bcb); + self.add_successors_to_worklists(bcb); return Some(bcb); } else { // Strip contexts with empty worklists from the top of the stack @@ -461,10 +461,10 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> { None } - pub fn extend_worklist(&mut self, bcb: BasicCoverageBlock) { - let Self { basic_coverage_blocks, .. } = *self; - let successors = &basic_coverage_blocks.successors[bcb]; + pub 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!( @@ -473,43 +473,44 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> { 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; } - for context in self.context_stack.iter_mut().rev() { - // 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 (some_successor_to_add, _) = - if let Some(loop_header) = context.loop_header { - if basic_coverage_blocks.dominates(loop_header, successor) { - (Some(successor), Some(loop_header)) - } else { - (None, None) - } - } else { - (Some(successor), None) - }; - - // 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 let Some(successor_to_add) = some_successor_to_add { - if basic_coverage_blocks.successors[successor_to_add].len() > 1 { - context.worklist.push_back(successor_to_add); - } else { - context.worklist.push_front(successor_to_add); + + // 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) } - 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); } } }