From ea3fb7bc2cd64fc124209e6f4ffcda8d053fd7ae Mon Sep 17 00:00:00 2001
From: Zalathar <Zalathar@users.noreply.github.com>
Date: Thu, 12 Oct 2023 12:32:21 +1100
Subject: [PATCH] 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 12fb897bd79c..8dc13117b9f4 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_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>, 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<BasicCoverageBlock>,
+    /// Worklist of BCBs to be processed in this context.
+    worklist: VecDeque<BasicCoverageBlock>,
 }
 
 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 @@ pub fn next(&mut self, basic_coverage_blocks: &CoverageGraph) -> Option<BasicCov
         );
 
         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 @@ pub fn next(&mut self, basic_coverage_blocks: &CoverageGraph) -> Option<BasicCov
                     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 @@ pub fn extend_worklist(
                 // 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 @@ pub fn extend_worklist(
                     } 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;
                 }