From f294181aa0d732e83031e66d5b50b76f705af308 Mon Sep 17 00:00:00 2001 From: Amanda Stjerna Date: Thu, 17 Apr 2025 12:11:13 +0200 Subject: [PATCH 1/2] Decouple SCC annotations from SCCs This rewires SCC annotations to have them be a separate, visitor-type data structure. It was broken out of #130227, which needed them to be able to remove unused annotations after computation without recomputing the SCCs themselves. As a drive-by it also removes some redundant code from the hot loop in SCC construction for a performance improvement. --- .../rustc_borrowck/src/constraints/mod.rs | 24 +-- .../rustc_borrowck/src/region_infer/mod.rs | 53 +++++- .../src/graph/scc/mod.rs | 132 ++++++------- .../src/graph/scc/tests.rs | 178 +++++++++++------- 4 files changed, 228 insertions(+), 159 deletions(-) diff --git a/compiler/rustc_borrowck/src/constraints/mod.rs b/compiler/rustc_borrowck/src/constraints/mod.rs index a52269df68289..514bbfe359b1d 100644 --- a/compiler/rustc_borrowck/src/constraints/mod.rs +++ b/compiler/rustc_borrowck/src/constraints/mod.rs @@ -7,7 +7,7 @@ use rustc_middle::ty::{RegionVid, TyCtxt, VarianceDiagInfo}; use rustc_span::Span; use tracing::{debug, instrument}; -use crate::region_infer::{ConstraintSccs, RegionDefinition, RegionTracker}; +use crate::region_infer::{AnnotatedSccs, ConstraintSccs, RegionDefinition, SccAnnotations}; use crate::type_check::Locations; use crate::universal_regions::UniversalRegions; @@ -61,12 +61,14 @@ impl<'tcx> OutlivesConstraintSet<'tcx> { &self, static_region: RegionVid, definitions: &IndexVec>, - ) -> ConstraintSccs { + ) -> AnnotatedSccs { let constraint_graph = self.graph(definitions.len()); let region_graph = &constraint_graph.region_graph(self, static_region); - ConstraintSccs::new_with_annotation(®ion_graph, |r| { - RegionTracker::new(r, &definitions[r]) - }) + let mut annotation_visitor = SccAnnotations::new(definitions); + ( + ConstraintSccs::new_with_annotation(®ion_graph, &mut annotation_visitor), + annotation_visitor.scc_to_annotation, + ) } /// This method handles Universe errors by rewriting the constraint @@ -79,12 +81,12 @@ impl<'tcx> OutlivesConstraintSet<'tcx> { /// eventually go away. /// /// For a more precise definition, see the documentation for - /// [`RegionTracker::has_incompatible_universes()`]. + /// [`crate::region_infer::RegionTracker`]. /// /// This edge case used to be handled during constraint propagation /// by iterating over the strongly connected components in the constraint /// graph while maintaining a set of bookkeeping mappings similar - /// to what is stored in `RegionTracker` and manually adding 'sttaic as + /// to what is stored in `RegionTracker` and manually adding 'static as /// needed. /// /// It was rewritten as part of the Polonius project with the goal of moving @@ -108,9 +110,9 @@ impl<'tcx> OutlivesConstraintSet<'tcx> { &mut self, universal_regions: &UniversalRegions<'tcx>, definitions: &IndexVec>, - ) -> ConstraintSccs { + ) -> AnnotatedSccs { let fr_static = universal_regions.fr_static; - let sccs = self.compute_sccs(fr_static, definitions); + let (sccs, annotations) = self.compute_sccs(fr_static, definitions); // Changed to `true` if we added any constraints to `self` and need to // recompute SCCs. @@ -124,7 +126,7 @@ impl<'tcx> OutlivesConstraintSet<'tcx> { continue; } - let annotation = sccs.annotation(scc); + let annotation = annotations[scc]; // If this SCC participates in a universe violation, // e.g. if it reaches a region with a universe smaller than @@ -154,7 +156,7 @@ impl<'tcx> OutlivesConstraintSet<'tcx> { self.compute_sccs(fr_static, definitions) } else { // If we didn't add any back-edges; no more work needs doing - sccs + (sccs, annotations) } } } diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs index a80d74d9e370a..dc796a97daee7 100644 --- a/compiler/rustc_borrowck/src/region_infer/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/mod.rs @@ -45,12 +45,13 @@ mod reverse_sccs; pub(crate) mod values; -pub(crate) type ConstraintSccs = Sccs; +pub(crate) type ConstraintSccs = Sccs; +pub(crate) type AnnotatedSccs = (ConstraintSccs, IndexVec); /// An annotation for region graph SCCs that tracks -/// the values of its elements. +/// the values of its elements. This annotates a single SCC. #[derive(Copy, Debug, Clone)] -pub struct RegionTracker { +pub(crate) struct RegionTracker { /// The largest universe of a placeholder reached from this SCC. /// This includes placeholders within this SCC. max_placeholder_universe_reached: UniverseIndex, @@ -95,6 +96,31 @@ impl scc::Annotation for RegionTracker { } } +/// A Visitor for SCC annotation construction. +pub(crate) struct SccAnnotations<'d, 'tcx, A: scc::Annotation> { + pub(crate) scc_to_annotation: IndexVec, + definitions: &'d IndexVec>, +} + +impl<'d, 'tcx, A: scc::Annotation> SccAnnotations<'d, 'tcx, A> { + pub(crate) fn new(definitions: &'d IndexVec>) -> Self { + Self { scc_to_annotation: IndexVec::new(), definitions } + } +} + +impl scc::Annotations + for SccAnnotations<'_, '_, RegionTracker> +{ + fn new(&self, element: RegionVid) -> RegionTracker { + RegionTracker::new(element, &self.definitions[element]) + } + + fn annotate_scc(&mut self, scc: ConstraintSccIndex, annotation: RegionTracker) { + let idx = self.scc_to_annotation.push(annotation); + assert!(idx == scc); + } +} + impl RegionTracker { pub(crate) fn new(rvid: RegionVid, definition: &RegionDefinition<'_>) -> Self { let (representative_is_placeholder, representative_is_existential) = match definition.origin @@ -166,6 +192,8 @@ pub struct RegionInferenceContext<'tcx> { /// compute the values of each region. constraint_sccs: ConstraintSccs, + scc_annotations: IndexVec, + /// Reverse of the SCC constraint graph -- i.e., an edge `A -> B` exists if /// `B: A`. This is used to compute the universal regions that are required /// to outlive a given SCC. Computed lazily. @@ -428,7 +456,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { .map(|info| RegionDefinition::new(info.universe, info.origin)) .collect(); - let constraint_sccs = + let (constraint_sccs, scc_annotations) = outlives_constraints.add_outlives_static(&universal_regions, &definitions); let constraints = Frozen::freeze(outlives_constraints); let constraint_graph = Frozen::freeze(constraints.graph(definitions.len())); @@ -455,6 +483,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { constraints, constraint_graph, constraint_sccs, + scc_annotations, rev_scc_graph: None, member_constraints, member_constraints_applied: Vec::new(), @@ -752,6 +781,10 @@ impl<'tcx> RegionInferenceContext<'tcx> { debug!(value = ?self.scc_values.region_value_str(scc_a)); } + fn scc_annotations(&self) -> &IndexVec { + &self.scc_annotations + } + /// Invoked for each `R0 member of [R1..Rn]` constraint. /// /// `scc` is the SCC containing R0, and `choice_regions` are the @@ -793,7 +826,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { // If the member region lives in a higher universe, we currently choose // the most conservative option by leaving it unchanged. - if !self.constraint_sccs().annotation(scc).min_universe().is_root() { + if !self.scc_annotations()[scc].min_universe().is_root() { return; } @@ -869,8 +902,8 @@ impl<'tcx> RegionInferenceContext<'tcx> { /// in `scc_a`. Used during constraint propagation, and only once /// the value of `scc_b` has been computed. fn universe_compatible(&self, scc_b: ConstraintSccIndex, scc_a: ConstraintSccIndex) -> bool { - let a_annotation = self.constraint_sccs().annotation(scc_a); - let b_annotation = self.constraint_sccs().annotation(scc_b); + let a_annotation = self.scc_annotations()[scc_a]; + let b_annotation = self.scc_annotations()[scc_b]; let a_universe = a_annotation.min_universe(); // If scc_b's declared universe is a subset of @@ -986,7 +1019,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { "lower_bound = {:?} r_scc={:?} universe={:?}", lower_bound, r_scc, - self.constraint_sccs.annotation(r_scc).min_universe() + self.scc_annotations()[r_scc].min_universe() ); // If the type test requires that `T: 'a` where `'a` is a // placeholder from another universe, that effectively requires @@ -1467,7 +1500,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { /// The minimum universe of any variable reachable from this /// SCC, inside or outside of it. fn scc_universe(&self, scc: ConstraintSccIndex) -> UniverseIndex { - self.constraint_sccs().annotation(scc).min_universe() + self.scc_annotations()[scc].min_universe() } /// Checks the final value for the free region `fr` to see if it @@ -2214,7 +2247,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { /// they *must* be equal (though not having the same repr does not /// mean they are unequal). fn scc_representative(&self, scc: ConstraintSccIndex) -> RegionVid { - self.constraint_sccs.annotation(scc).representative + self.scc_annotations()[scc].representative } pub(crate) fn liveness_constraints(&self) -> &LivenessValues { diff --git a/compiler/rustc_data_structures/src/graph/scc/mod.rs b/compiler/rustc_data_structures/src/graph/scc/mod.rs index e7c4ea3daae45..c0fe9e8ff93e9 100644 --- a/compiler/rustc_data_structures/src/graph/scc/mod.rs +++ b/compiler/rustc_data_structures/src/graph/scc/mod.rs @@ -13,7 +13,7 @@ use std::fmt::Debug; use std::ops::Range; use rustc_index::{Idx, IndexSlice, IndexVec}; -use tracing::{debug, instrument}; +use tracing::{debug, instrument, trace}; use crate::fx::FxHashSet; use crate::graph::vec_graph::VecGraph; @@ -48,6 +48,20 @@ pub trait Annotation: Debug + Copy { } } +/// An accumulator for annotations. +pub trait Annotations { + fn new(&self, element: N) -> A; + fn annotate_scc(&mut self, scc: S, annotation: A); +} + +/// The nil annotation accumulator, which does nothing. +impl Annotations for () { + fn new(&self, _element: N) -> () { + () + } + fn annotate_scc(&mut self, _scc: S, _annotation: ()) {} +} + /// The empty annotation, which does nothing. impl Annotation for () { fn merge_reached(self, _other: Self) -> Self { @@ -62,23 +76,20 @@ impl Annotation for () { /// the index type for the graph nodes and `S` is the index type for /// the SCCs. We can map from each node to the SCC that it /// participates in, and we also have the successors of each SCC. -pub struct Sccs { +pub struct Sccs { /// For each node, what is the SCC index of the SCC to which it /// belongs. scc_indices: IndexVec, /// Data about all the SCCs. - scc_data: SccData, + scc_data: SccData, } /// Information about an invidividual SCC node. -struct SccDetails { +struct SccDetails { /// For this SCC, the range of `all_successors` where its /// successors can be found. range: Range, - - /// User-specified metadata about the SCC. - annotation: A, } // The name of this struct should discourage you from making it public and leaking @@ -87,10 +98,10 @@ struct SccDetails { // is difficult when it's publicly inspectable. // // Obey the law of Demeter! -struct SccData { +struct SccData { /// Maps SCC indices to their metadata, including /// offsets into `all_successors`. - scc_details: IndexVec>, + scc_details: IndexVec, /// Contains the successors for all the Sccs, concatenated. The /// range of indices corresponding to a given SCC is found in its @@ -98,24 +109,18 @@ struct SccData { all_successors: Vec, } -impl Sccs { +impl Sccs { /// Compute SCCs without annotations. pub fn new(graph: &impl Successors) -> Self { - Self::new_with_annotation(graph, |_| ()) + Self::new_with_annotation(graph, &mut ()) } -} -impl Sccs { /// Compute SCCs and annotate them with a user-supplied annotation - pub fn new_with_annotation A>( + pub fn new_with_annotation>( graph: &impl Successors, - to_annotation: F, + annotations: &mut AA, ) -> Self { - SccsConstruction::construct(graph, to_annotation) - } - - pub fn annotation(&self, scc: S) -> A { - self.scc_data.annotation(scc) + SccsConstruction::construct(graph, annotations) } pub fn scc_indices(&self) -> &IndexSlice { @@ -136,7 +141,13 @@ impl Sccs { pub fn all_sccs(&self) -> impl Iterator + 'static { (0..self.scc_data.len()).map(S::new) } - + /* + /// Returns an iterator over the SCC annotations in the graph + /// The order is the same as `all_sccs()`, dependency order. + pub fn all_annotations(&self, annotations: &A) -> impl Iterator + use<'_, N, S, A> { + self.all_sccs().map(|scc| (scc, self.annotation(scc))) + } + */ /// Returns the SCC to which a node `r` belongs. pub fn scc(&self, r: N) -> S { self.scc_indices[r] @@ -160,7 +171,7 @@ impl Sccs { } } -impl DirectedGraph for Sccs { +impl DirectedGraph for Sccs { type Node = S; fn num_nodes(&self) -> usize { @@ -168,19 +179,19 @@ impl DirectedGraph for Sccs { } } -impl NumEdges for Sccs { +impl NumEdges for Sccs { fn num_edges(&self) -> usize { self.scc_data.all_successors.len() } } -impl Successors for Sccs { +impl Successors for Sccs { fn successors(&self, node: S) -> impl Iterator { self.successors(node).iter().cloned() } } -impl SccData { +impl SccData { /// Number of SCCs, fn len(&self) -> usize { self.scc_details.len() @@ -192,9 +203,8 @@ impl SccData { } /// Creates a new SCC with `successors` as its successors and - /// the maximum weight of its internal nodes `scc_max_weight` and /// returns the resulting index. - fn create_scc(&mut self, successors: impl IntoIterator, annotation: A) -> S { + fn create_scc(&mut self, successors: impl IntoIterator) -> S { // Store the successors on `scc_successors_vec`, remembering // the range of indices. let all_successors_start = self.all_successors.len(); @@ -202,28 +212,23 @@ impl SccData { let all_successors_end = self.all_successors.len(); debug!( - "create_scc({:?}) successors={:?}, annotation={:?}", + "create_scc({:?}) successors={:?}", self.len(), &self.all_successors[all_successors_start..all_successors_end], - annotation ); let range = all_successors_start..all_successors_end; - let metadata = SccDetails { range, annotation }; + let metadata = SccDetails { range }; self.scc_details.push(metadata) } - - fn annotation(&self, scc: S) -> A { - self.scc_details[scc].annotation - } } -struct SccsConstruction<'c, G, S, A, F> +struct SccsConstruction<'c, 'a, G, S, A, AA> where G: DirectedGraph + Successors, S: Idx, A: Annotation, - F: Fn(G::Node) -> A, + AA: Annotations, { graph: &'c G, @@ -247,11 +252,9 @@ where /// around between successors to amortize memory allocation costs. duplicate_set: FxHashSet, - scc_data: SccData, + scc_data: SccData, - /// A function that constructs an initial SCC annotation - /// out of a single node. - to_annotation: F, + annotations: &'a mut AA, } #[derive(Copy, Clone, Debug)] @@ -299,12 +302,12 @@ enum WalkReturn { Complete { scc_index: S, annotation: A }, } -impl<'c, G, S, A, F> SccsConstruction<'c, G, S, A, F> +impl<'c, 'a, G, S, A, AA> SccsConstruction<'c, 'a, G, S, A, AA> where G: DirectedGraph + Successors, S: Idx, - F: Fn(G::Node) -> A, A: Annotation, + AA: Annotations, { /// Identifies SCCs in the graph `G` and computes the resulting /// DAG. This uses a variant of [Tarjan's @@ -320,7 +323,7 @@ where /// Additionally, we keep track of a current annotation of the SCC. /// /// [wikipedia]: https://bit.ly/2EZIx84 - fn construct(graph: &'c G, to_annotation: F) -> Sccs { + fn construct(graph: &'c G, annotations: &'a mut AA) -> Sccs { let num_nodes = graph.num_nodes(); let mut this = Self { @@ -330,7 +333,7 @@ where successors_stack: Vec::new(), scc_data: SccData { scc_details: IndexVec::new(), all_successors: Vec::new() }, duplicate_set: FxHashSet::default(), - to_annotation, + annotations, }; let scc_indices = graph @@ -408,7 +411,7 @@ where // a potentially derived version of the root state for non-root nodes in the chain. let (root_state, assigned_state) = { loop { - debug!("find_state(r = {node:?} in state {:?})", self.node_states[node]); + trace!("find_state(r = {node:?} in state {:?})", self.node_states[node]); match self.node_states[node] { // This must have been the first and only state since it is unexplored*; // no update needed! * Unless there is a bug :') @@ -482,7 +485,7 @@ where if previous_node == node { return root_state; } - debug!("Compressing {node:?} down to {previous_node:?} with state {assigned_state:?}"); + trace!("Compressing {node:?} down to {previous_node:?} with state {assigned_state:?}"); // Update to previous node in the link. match self.node_states[previous_node] { @@ -507,9 +510,9 @@ where /// Call this method when `inspect_node` has returned `None`. Having the /// caller decide avoids mutual recursion between the two methods and allows /// us to maintain an allocated stack for nodes on the path between calls. - #[instrument(skip(self, initial), level = "debug")] + #[instrument(skip(self, initial), level = "trace")] fn walk_unvisited_node(&mut self, initial: G::Node) -> WalkReturn { - debug!("Walk unvisited node: {initial:?}"); + trace!("Walk unvisited node: {initial:?}"); struct VisitingNodeFrame { node: G::Node, successors: Option, @@ -537,7 +540,7 @@ where successors_len: 0, min_cycle_root: initial, successor_node: initial, - current_component_annotation: (self.to_annotation)(initial), + current_component_annotation: self.annotations.new(initial), }]; let mut return_value = None; @@ -556,11 +559,7 @@ where let node = *node; let depth = *depth; - // node is definitely in the current component, add it to the annotation. - if node != initial { - current_component_annotation.update_scc((self.to_annotation)(node)); - } - debug!( + trace!( "Visiting {node:?} at depth {depth:?}, annotation: {current_component_annotation:?}" ); @@ -568,7 +567,7 @@ where Some(successors) => successors, None => { // This None marks that we still have the initialize this node's frame. - debug!(?depth, ?node); + trace!(?depth, ?node); debug_assert_matches!(self.node_states[node], NodeState::NotVisited); @@ -598,7 +597,7 @@ where return_value.take().into_iter().map(|walk| (*successor_node, Some(walk))); let successor_walk = successors.map(|successor_node| { - debug!(?node, ?successor_node); + trace!(?node, ?successor_node); (successor_node, self.inspect_node(successor_node)) }); for (successor_node, walk) in returned_walk.chain(successor_walk) { @@ -609,13 +608,13 @@ where min_depth: successor_min_depth, annotation: successor_annotation, }) => { - debug!( + trace!( "Cycle found from {node:?}, minimum depth: {successor_min_depth:?}, annotation: {successor_annotation:?}" ); // Track the minimum depth we can reach. assert!(successor_min_depth <= depth); if successor_min_depth < *min_depth { - debug!(?node, ?successor_min_depth); + trace!(?node, ?successor_min_depth); *min_depth = successor_min_depth; *min_cycle_root = successor_node; } @@ -627,20 +626,20 @@ where scc_index: successor_scc_index, annotation: successor_annotation, }) => { - debug!( + trace!( "Complete; {node:?} is root of complete-visited SCC idx {successor_scc_index:?} with annotation {successor_annotation:?}" ); // Push the completed SCC indices onto // the `successors_stack` for later. - debug!(?node, ?successor_scc_index); + trace!(?node, ?successor_scc_index); successors_stack.push(successor_scc_index); current_component_annotation.update_reachable(successor_annotation); } // `node` has no more (direct) successors; search recursively. None => { let depth = depth + 1; - debug!("Recursing down into {successor_node:?} at depth {depth:?}"); - debug!(?depth, ?successor_node); + trace!("Recursing down into {successor_node:?} at depth {depth:?}"); + trace!(?depth, ?successor_node); // Remember which node the return value will come from. frame.successor_node = successor_node; // Start a new stack frame, then step into it. @@ -652,14 +651,14 @@ where min_depth: depth, min_cycle_root: successor_node, successor_node, - current_component_annotation: (self.to_annotation)(successor_node), + current_component_annotation: self.annotations.new(successor_node), }); continue 'recurse; } } } - debug!("Finished walk from {node:?} with annotation: {current_component_annotation:?}"); + trace!("Finished walk from {node:?} with annotation: {current_component_annotation:?}"); // Completed walk, remove `node` from the stack. let r = self.node_stack.pop(); @@ -691,8 +690,9 @@ where debug!("Creating SCC rooted in {node:?} with successor {:?}", frame.successor_node); - let scc_index = - self.scc_data.create_scc(deduplicated_successors, current_component_annotation); + let scc_index = self.scc_data.create_scc(deduplicated_successors); + + self.annotations.annotate_scc(scc_index, current_component_annotation); self.node_states[node] = NodeState::InCycle { scc_index, annotation: current_component_annotation }; diff --git a/compiler/rustc_data_structures/src/graph/scc/tests.rs b/compiler/rustc_data_structures/src/graph/scc/tests.rs index 373f87bfdbcfa..e388e7b6680af 100644 --- a/compiler/rustc_data_structures/src/graph/scc/tests.rs +++ b/compiler/rustc_data_structures/src/graph/scc/tests.rs @@ -5,8 +5,28 @@ use crate::graph::tests::TestGraph; #[derive(Copy, Clone, Debug)] struct MaxReached(usize); -type UsizeSccs = Sccs; -type MaxReachedSccs = Sccs; +struct Maxes(IndexVec, fn(usize) -> usize); +type UsizeSccs = Sccs; + +impl Annotations for Maxes { + fn new(&self, element: usize) -> MaxReached { + MaxReached(self.1(element)) + } + + fn annotate_scc(&mut self, scc: usize, annotation: MaxReached) { + let i = self.0.push(annotation); + assert!(i == scc); + } +} + +impl Maxes { + fn annotation(&self, scc: usize) -> MaxReached { + self.0[scc] + } + fn new(mapping: fn(usize) -> usize) -> Self { + Self(IndexVec::new(), mapping) + } +} impl Annotation for MaxReached { fn merge_scc(self, other: Self) -> Self { @@ -14,7 +34,7 @@ impl Annotation for MaxReached { } fn merge_reached(self, other: Self) -> Self { - self.merge_scc(other) + Self(std::cmp::max(other.0, self.0)) } } @@ -24,17 +44,29 @@ impl PartialEq for MaxReached { } } -impl MaxReached { - fn from_usize(nr: usize) -> Self { - Self(nr) - } -} - #[derive(Copy, Clone, Debug)] struct MinMaxIn { min: usize, max: usize, } +struct MinMaxes(IndexVec, fn(usize) -> MinMaxIn); + +impl MinMaxes { + fn annotation(&self, scc: usize) -> MinMaxIn { + self.0[scc] + } +} + +impl Annotations for MinMaxes { + fn new(&self, element: usize) -> MinMaxIn { + self.1(element) + } + + fn annotate_scc(&mut self, scc: usize, annotation: MinMaxIn) { + let i = self.0.push(annotation); + assert!(i == scc); + } +} impl Annotation for MinMaxIn { fn merge_scc(self, other: Self) -> Self { @@ -261,67 +293,68 @@ fn bench_sccc(b: &mut test::Bencher) { #[test] fn test_max_self_loop() { let graph = TestGraph::new(0, &[(0, 0)]); - let sccs: MaxReachedSccs = - Sccs::new_with_annotation(&graph, |n| if n == 0 { MaxReached(17) } else { MaxReached(0) }); - assert_eq!(sccs.annotation(0), 17); + let mut annotations = Maxes(IndexVec::new(), |n| if n == 0 { 17 } else { 0 }); + Sccs::new_with_annotation(&graph, &mut annotations); + assert_eq!(annotations.0[0], 17); } #[test] fn test_max_branch() { let graph = TestGraph::new(0, &[(0, 1), (0, 2), (1, 3), (2, 4)]); - let sccs: MaxReachedSccs = Sccs::new_with_annotation(&graph, MaxReached::from_usize); - assert_eq!(sccs.annotation(sccs.scc(0)), 4); - assert_eq!(sccs.annotation(sccs.scc(1)), 3); - assert_eq!(sccs.annotation(sccs.scc(2)), 4); + let mut annotations = Maxes(IndexVec::new(), |n| n); + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); + assert_eq!(annotations.0[sccs.scc(0)], 4); + assert_eq!(annotations.0[sccs.scc(1)], 3); + assert_eq!(annotations.0[sccs.scc(2)], 4); } + #[test] fn test_single_cycle_max() { let graph = TestGraph::new(0, &[(0, 2), (2, 3), (2, 4), (4, 1), (1, 2)]); - let sccs: MaxReachedSccs = Sccs::new_with_annotation(&graph, MaxReached::from_usize); - assert_eq!(sccs.annotation(sccs.scc(2)), 4); - assert_eq!(sccs.annotation(sccs.scc(0)), 4); -} - -#[test] -fn test_simple_cycle_max() { - let graph = TestGraph::new(0, &[(0, 1), (1, 2), (2, 0)]); - let sccs: MaxReachedSccs = Sccs::new_with_annotation(&graph, MaxReached::from_usize); - assert_eq!(sccs.num_sccs(), 1); + let mut annotations = Maxes(IndexVec::new(), |n| n); + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); + assert_eq!(annotations.0[sccs.scc(2)], 4); + assert_eq!(annotations.0[sccs.scc(0)], 4); } #[test] fn test_double_cycle_max() { let graph = TestGraph::new(0, &[(0, 1), (1, 2), (1, 4), (2, 3), (2, 4), (3, 5), (4, 1), (5, 4)]); - let sccs: MaxReachedSccs = - Sccs::new_with_annotation(&graph, |n| if n == 5 { MaxReached(2) } else { MaxReached(1) }); + let mut annotations = Maxes(IndexVec::new(), |n| if n == 5 { 2 } else { 1 }); - assert_eq!(sccs.annotation(sccs.scc(0)).0, 2); + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); + + assert_eq!(annotations.0[sccs.scc(0)].0, 2); } #[test] fn test_bug_minimised() { let graph = TestGraph::new(0, &[(0, 3), (0, 1), (3, 2), (2, 3), (1, 4), (4, 5), (5, 4)]); - let sccs: MaxReachedSccs = Sccs::new_with_annotation(&graph, |n| match n { - 3 => MaxReached(1), - _ => MaxReached(0), + let mut annotations = Maxes(IndexVec::new(), |n| match n { + 3 => 1, + _ => 0, }); - assert_eq!(sccs.annotation(sccs.scc(2)), 1); - assert_eq!(sccs.annotation(sccs.scc(1)), 0); - assert_eq!(sccs.annotation(sccs.scc(4)), 0); + + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); + assert_eq!(annotations.annotation(sccs.scc(2)), 1); + assert_eq!(annotations.annotation(sccs.scc(1)), 0); + assert_eq!(annotations.annotation(sccs.scc(4)), 0); } #[test] fn test_bug_max_leak_minimised() { let graph = TestGraph::new(0, &[(0, 1), (0, 2), (1, 3), (3, 0), (3, 4), (4, 3)]); - let sccs: MaxReachedSccs = Sccs::new_with_annotation(&graph, |w| match w { - 4 => MaxReached(1), - _ => MaxReached(0), + let mut annotations = Maxes(IndexVec::new(), |w| match w { + 4 => 1, + _ => 0, }); - assert_eq!(sccs.annotation(sccs.scc(2)), 0); - assert_eq!(sccs.annotation(sccs.scc(3)), 1); - assert_eq!(sccs.annotation(sccs.scc(0)), 1); + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); + + assert_eq!(annotations.annotation(sccs.scc(2)), 0); + assert_eq!(annotations.annotation(sccs.scc(3)), 1); + assert_eq!(annotations.annotation(sccs.scc(0)), 1); } #[test] @@ -369,48 +402,49 @@ fn test_bug_max_leak() { (23, 24), ], ); - let sccs: MaxReachedSccs = Sccs::new_with_annotation(&graph, |w| match w { - 22 => MaxReached(1), - 24 => MaxReached(2), - 27 => MaxReached(2), - _ => MaxReached(0), + let mut annotations = Maxes::new(|w| match w { + 22 => 1, + 24 => 2, + 27 => 2, + _ => 0, }); - - assert_eq!(sccs.annotation(sccs.scc(2)), 0); - assert_eq!(sccs.annotation(sccs.scc(7)), 0); - assert_eq!(sccs.annotation(sccs.scc(8)), 2); - assert_eq!(sccs.annotation(sccs.scc(23)), 2); - assert_eq!(sccs.annotation(sccs.scc(3)), 2); - assert_eq!(sccs.annotation(sccs.scc(0)), 2); + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); + + assert_eq!(annotations.annotation(sccs.scc(2)), 0); + assert_eq!(annotations.annotation(sccs.scc(7)), 0); + assert_eq!(annotations.annotation(sccs.scc(8)), 2); + assert_eq!(annotations.annotation(sccs.scc(23)), 2); + assert_eq!(annotations.annotation(sccs.scc(3)), 2); + assert_eq!(annotations.annotation(sccs.scc(0)), 2); } #[test] fn test_bug_max_zero_stick_shape() { let graph = TestGraph::new(0, &[(0, 1), (1, 2), (2, 3), (3, 2), (3, 4)]); - - let sccs: MaxReachedSccs = Sccs::new_with_annotation(&graph, |w| match w { - 4 => MaxReached(1), - _ => MaxReached(0), + let mut annotations = Maxes::new(|w| match w { + 4 => 1, + _ => 0, }); + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); - assert_eq!(sccs.annotation(sccs.scc(0)), 1); - assert_eq!(sccs.annotation(sccs.scc(1)), 1); - assert_eq!(sccs.annotation(sccs.scc(2)), 1); - assert_eq!(sccs.annotation(sccs.scc(3)), 1); - assert_eq!(sccs.annotation(sccs.scc(4)), 1); + assert_eq!(annotations.annotation(sccs.scc(0)), 1); + assert_eq!(annotations.annotation(sccs.scc(1)), 1); + assert_eq!(annotations.annotation(sccs.scc(2)), 1); + assert_eq!(annotations.annotation(sccs.scc(3)), 1); + assert_eq!(annotations.annotation(sccs.scc(4)), 1); } #[test] fn test_min_max_in() { let graph = TestGraph::new(0, &[(0, 1), (0, 2), (1, 3), (3, 0), (3, 4), (4, 3), (3, 5)]); - let sccs: Sccs = - Sccs::new_with_annotation(&graph, |w| MinMaxIn { min: w, max: w }); - - assert_eq!(sccs.annotation(sccs.scc(2)).min, 2); - assert_eq!(sccs.annotation(sccs.scc(2)).max, 2); - assert_eq!(sccs.annotation(sccs.scc(0)).min, 0); - assert_eq!(sccs.annotation(sccs.scc(0)).max, 4); - assert_eq!(sccs.annotation(sccs.scc(3)).min, 0); - assert_eq!(sccs.annotation(sccs.scc(3)).max, 4); - assert_eq!(sccs.annotation(sccs.scc(5)).min, 5); + let mut annotations = MinMaxes(IndexVec::new(), |w| MinMaxIn { min: w, max: w }); + let sccs = Sccs::new_with_annotation(&graph, &mut annotations); + + assert_eq!(annotations.annotation(sccs.scc(2)).min, 2); + assert_eq!(annotations.annotation(sccs.scc(2)).max, 2); + assert_eq!(annotations.annotation(sccs.scc(0)).min, 0); + assert_eq!(annotations.annotation(sccs.scc(0)).max, 4); + assert_eq!(annotations.annotation(sccs.scc(3)).min, 0); + assert_eq!(annotations.annotation(sccs.scc(3)).max, 4); + assert_eq!(annotations.annotation(sccs.scc(5)).min, 5); } From 72911536cd371c101bc8417027c35c06e7350588 Mon Sep 17 00:00:00 2001 From: Amanda Stjerna Date: Fri, 25 Apr 2025 14:29:21 +0200 Subject: [PATCH 2/2] Use associated types for SCC annotations, per code review suggestion --- .../rustc_borrowck/src/region_infer/mod.rs | 23 +++--- .../src/graph/scc/mod.rs | 70 +++++++++---------- .../src/graph/scc/tests.rs | 10 ++- 3 files changed, 51 insertions(+), 52 deletions(-) diff --git a/compiler/rustc_borrowck/src/region_infer/mod.rs b/compiler/rustc_borrowck/src/region_infer/mod.rs index dc796a97daee7..df03690d049d2 100644 --- a/compiler/rustc_borrowck/src/region_infer/mod.rs +++ b/compiler/rustc_borrowck/src/region_infer/mod.rs @@ -108,9 +108,7 @@ impl<'d, 'tcx, A: scc::Annotation> SccAnnotations<'d, 'tcx, A> { } } -impl scc::Annotations - for SccAnnotations<'_, '_, RegionTracker> -{ +impl scc::Annotations for SccAnnotations<'_, '_, RegionTracker> { fn new(&self, element: RegionVid) -> RegionTracker { RegionTracker::new(element, &self.definitions[element]) } @@ -119,6 +117,9 @@ impl scc::Annotations let idx = self.scc_to_annotation.push(annotation); assert!(idx == scc); } + + type Ann = RegionTracker; + type SccIdx = ConstraintSccIndex; } impl RegionTracker { @@ -781,10 +782,6 @@ impl<'tcx> RegionInferenceContext<'tcx> { debug!(value = ?self.scc_values.region_value_str(scc_a)); } - fn scc_annotations(&self) -> &IndexVec { - &self.scc_annotations - } - /// Invoked for each `R0 member of [R1..Rn]` constraint. /// /// `scc` is the SCC containing R0, and `choice_regions` are the @@ -826,7 +823,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { // If the member region lives in a higher universe, we currently choose // the most conservative option by leaving it unchanged. - if !self.scc_annotations()[scc].min_universe().is_root() { + if !self.scc_universe(scc).is_root() { return; } @@ -902,8 +899,8 @@ impl<'tcx> RegionInferenceContext<'tcx> { /// in `scc_a`. Used during constraint propagation, and only once /// the value of `scc_b` has been computed. fn universe_compatible(&self, scc_b: ConstraintSccIndex, scc_a: ConstraintSccIndex) -> bool { - let a_annotation = self.scc_annotations()[scc_a]; - let b_annotation = self.scc_annotations()[scc_b]; + let a_annotation = self.scc_annotations[scc_a]; + let b_annotation = self.scc_annotations[scc_b]; let a_universe = a_annotation.min_universe(); // If scc_b's declared universe is a subset of @@ -1019,7 +1016,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { "lower_bound = {:?} r_scc={:?} universe={:?}", lower_bound, r_scc, - self.scc_annotations()[r_scc].min_universe() + self.scc_universe(r_scc) ); // If the type test requires that `T: 'a` where `'a` is a // placeholder from another universe, that effectively requires @@ -1500,7 +1497,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { /// The minimum universe of any variable reachable from this /// SCC, inside or outside of it. fn scc_universe(&self, scc: ConstraintSccIndex) -> UniverseIndex { - self.scc_annotations()[scc].min_universe() + self.scc_annotations[scc].min_universe() } /// Checks the final value for the free region `fr` to see if it @@ -2247,7 +2244,7 @@ impl<'tcx> RegionInferenceContext<'tcx> { /// they *must* be equal (though not having the same repr does not /// mean they are unequal). fn scc_representative(&self, scc: ConstraintSccIndex) -> RegionVid { - self.scc_annotations()[scc].representative + self.scc_annotations[scc].representative } pub(crate) fn liveness_constraints(&self) -> &LivenessValues { diff --git a/compiler/rustc_data_structures/src/graph/scc/mod.rs b/compiler/rustc_data_structures/src/graph/scc/mod.rs index c0fe9e8ff93e9..518817ea0f53a 100644 --- a/compiler/rustc_data_structures/src/graph/scc/mod.rs +++ b/compiler/rustc_data_structures/src/graph/scc/mod.rs @@ -10,6 +10,7 @@ use std::assert_matches::debug_assert_matches; use std::fmt::Debug; +use std::marker::PhantomData; use std::ops::Range; use rustc_index::{Idx, IndexSlice, IndexVec}; @@ -49,16 +50,21 @@ pub trait Annotation: Debug + Copy { } /// An accumulator for annotations. -pub trait Annotations { - fn new(&self, element: N) -> A; - fn annotate_scc(&mut self, scc: S, annotation: A); +pub trait Annotations { + type Ann: Annotation; + type SccIdx: Idx + Ord; + + fn new(&self, element: N) -> Self::Ann; + fn annotate_scc(&mut self, scc: Self::SccIdx, annotation: Self::Ann); } /// The nil annotation accumulator, which does nothing. -impl Annotations for () { - fn new(&self, _element: N) -> () { - () - } +struct NoAnnotations(PhantomData); + +impl Annotations for NoAnnotations { + type SccIdx = S; + type Ann = (); + fn new(&self, _element: N) {} fn annotate_scc(&mut self, _scc: S, _annotation: ()) {} } @@ -112,13 +118,13 @@ struct SccData { impl Sccs { /// Compute SCCs without annotations. pub fn new(graph: &impl Successors) -> Self { - Self::new_with_annotation(graph, &mut ()) + Self::new_with_annotation(graph, &mut NoAnnotations(PhantomData::)) } /// Compute SCCs and annotate them with a user-supplied annotation - pub fn new_with_annotation>( + pub fn new_with_annotation>( graph: &impl Successors, - annotations: &mut AA, + annotations: &mut A, ) -> Self { SccsConstruction::construct(graph, annotations) } @@ -141,13 +147,7 @@ impl Sccs { pub fn all_sccs(&self) -> impl Iterator + 'static { (0..self.scc_data.len()).map(S::new) } - /* - /// Returns an iterator over the SCC annotations in the graph - /// The order is the same as `all_sccs()`, dependency order. - pub fn all_annotations(&self, annotations: &A) -> impl Iterator + use<'_, N, S, A> { - self.all_sccs().map(|scc| (scc, self.annotation(scc))) - } - */ + /// Returns the SCC to which a node `r` belongs. pub fn scc(&self, r: N) -> S { self.scc_indices[r] @@ -223,19 +223,17 @@ impl SccData { } } -struct SccsConstruction<'c, 'a, G, S, A, AA> +struct SccsConstruction<'c, 'a, G, A> where G: DirectedGraph + Successors, - S: Idx, - A: Annotation, - AA: Annotations, + A: Annotations, { graph: &'c G, /// The state of each node; used during walk to record the stack /// and after walk to record what cycle each node ended up being /// in. - node_states: IndexVec>, + node_states: IndexVec>, /// The stack of nodes that we are visiting as part of the DFS. node_stack: Vec, @@ -244,21 +242,21 @@ where /// position in this stack, and when we encounter a successor SCC, /// we push it on the stack. When we complete an SCC, we can pop /// everything off the stack that was found along the way. - successors_stack: Vec, + successors_stack: Vec, /// A set used to strip duplicates. As we accumulate successors /// into the successors_stack, we sometimes get duplicate entries. /// We use this set to remove those -- we also keep its storage /// around between successors to amortize memory allocation costs. - duplicate_set: FxHashSet, + duplicate_set: FxHashSet, - scc_data: SccData, + scc_data: SccData, - annotations: &'a mut AA, + annotations: &'a mut A, } #[derive(Copy, Clone, Debug)] -enum NodeState { +enum NodeState { /// This node has not yet been visited as part of the DFS. /// /// After SCC construction is complete, this state ought to be @@ -289,7 +287,7 @@ enum NodeState { /// The state of walking a given node. #[derive(Copy, Clone, Debug)] -enum WalkReturn { +enum WalkReturn { /// The walk found a cycle, but the entire component is not known to have /// been fully walked yet. We only know the minimum depth of this /// component in a minimum spanning tree of the graph. This component @@ -302,12 +300,10 @@ enum WalkReturn { Complete { scc_index: S, annotation: A }, } -impl<'c, 'a, G, S, A, AA> SccsConstruction<'c, 'a, G, S, A, AA> +impl<'c, 'a, G, A> SccsConstruction<'c, 'a, G, A> where G: DirectedGraph + Successors, - S: Idx, - A: Annotation, - AA: Annotations, + A: Annotations, { /// Identifies SCCs in the graph `G` and computes the resulting /// DAG. This uses a variant of [Tarjan's @@ -323,7 +319,7 @@ where /// Additionally, we keep track of a current annotation of the SCC. /// /// [wikipedia]: https://bit.ly/2EZIx84 - fn construct(graph: &'c G, annotations: &'a mut AA) -> Sccs { + fn construct(graph: &'c G, annotations: &'a mut A) -> Sccs { let num_nodes = graph.num_nodes(); let mut this = Self { @@ -349,7 +345,7 @@ where Sccs { scc_indices, scc_data: this.scc_data } } - fn start_walk_from(&mut self, node: G::Node) -> WalkReturn { + fn start_walk_from(&mut self, node: G::Node) -> WalkReturn { self.inspect_node(node).unwrap_or_else(|| self.walk_unvisited_node(node)) } @@ -365,7 +361,7 @@ where /// Otherwise, we are looking at a node that has already been /// completely visited. We therefore return `WalkReturn::Complete` /// with its associated SCC index. - fn inspect_node(&mut self, node: G::Node) -> Option> { + fn inspect_node(&mut self, node: G::Node) -> Option> { Some(match self.find_state(node) { NodeState::InCycle { scc_index, annotation } => { WalkReturn::Complete { scc_index, annotation } @@ -388,7 +384,7 @@ where /// of `r2` (and updates `r` to reflect current result). This is /// basically the "find" part of a standard union-find algorithm /// (with path compression). - fn find_state(&mut self, mut node: G::Node) -> NodeState { + fn find_state(&mut self, mut node: G::Node) -> NodeState { // To avoid recursion we temporarily reuse the `parent` of each // InCycleWith link to encode a downwards link while compressing // the path. After we have found the root or deepest node being @@ -511,7 +507,7 @@ where /// caller decide avoids mutual recursion between the two methods and allows /// us to maintain an allocated stack for nodes on the path between calls. #[instrument(skip(self, initial), level = "trace")] - fn walk_unvisited_node(&mut self, initial: G::Node) -> WalkReturn { + fn walk_unvisited_node(&mut self, initial: G::Node) -> WalkReturn { trace!("Walk unvisited node: {initial:?}"); struct VisitingNodeFrame { node: G::Node, diff --git a/compiler/rustc_data_structures/src/graph/scc/tests.rs b/compiler/rustc_data_structures/src/graph/scc/tests.rs index e388e7b6680af..8f04baf51f355 100644 --- a/compiler/rustc_data_structures/src/graph/scc/tests.rs +++ b/compiler/rustc_data_structures/src/graph/scc/tests.rs @@ -8,7 +8,7 @@ struct MaxReached(usize); struct Maxes(IndexVec, fn(usize) -> usize); type UsizeSccs = Sccs; -impl Annotations for Maxes { +impl Annotations for Maxes { fn new(&self, element: usize) -> MaxReached { MaxReached(self.1(element)) } @@ -17,6 +17,9 @@ impl Annotations for Maxes { let i = self.0.push(annotation); assert!(i == scc); } + + type Ann = MaxReached; + type SccIdx = usize; } impl Maxes { @@ -57,7 +60,7 @@ impl MinMaxes { } } -impl Annotations for MinMaxes { +impl Annotations for MinMaxes { fn new(&self, element: usize) -> MinMaxIn { self.1(element) } @@ -66,6 +69,9 @@ impl Annotations for MinMaxes { let i = self.0.push(annotation); assert!(i == scc); } + + type Ann = MinMaxIn; + type SccIdx = usize; } impl Annotation for MinMaxIn {