From c6111c0e80f9b323c80b149542ca48e5634b48df Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 29 Aug 2024 10:54:08 +1000 Subject: [PATCH 1/4] Remove the `'body` lifetime on `FilterInformation`. It's not needed. --- compiler/rustc_mir_transform/src/dest_prop.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs index ed924761892..1022d218f00 100644 --- a/compiler/rustc_mir_transform/src/dest_prop.rs +++ b/compiler/rustc_mir_transform/src/dest_prop.rs @@ -359,8 +359,8 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Merger<'a, 'tcx> { // // This section enforces bullet point 2 -struct FilterInformation<'a, 'body, 'alloc, 'tcx> { - body: &'body Body<'tcx>, +struct FilterInformation<'a, 'alloc, 'tcx> { + body: &'a Body<'tcx>, points: &'a DenseLocationMap, live: &'a SparseIntervalMatrix, candidates: &'a mut Candidates<'alloc>, @@ -446,7 +446,7 @@ enum CandidateFilter { Remove, } -impl<'a, 'body, 'alloc, 'tcx> FilterInformation<'a, 'body, 'alloc, 'tcx> { +impl<'a, 'alloc, 'tcx> FilterInformation<'a, 'alloc, 'tcx> { /// Filters the set of candidates to remove those that conflict. /// /// The steps we take are exactly those that are outlined at the top of the file. For each @@ -464,12 +464,12 @@ impl<'a, 'body, 'alloc, 'tcx> FilterInformation<'a, 'body, 'alloc, 'tcx> { /// before the statement/terminator will correctly report locals that are read in the /// statement/terminator to be live. We are additionally conservative by treating all written to /// locals as also being read from. - fn filter_liveness<'b>( + fn filter_liveness( candidates: &mut Candidates<'alloc>, points: &DenseLocationMap, live: &SparseIntervalMatrix, - write_info_alloc: &'alloc mut WriteInfo, - body: &'b Body<'tcx>, + write_info: &'alloc mut WriteInfo, + body: &Body<'tcx>, ) { let mut this = FilterInformation { body, @@ -478,7 +478,7 @@ impl<'a, 'body, 'alloc, 'tcx> FilterInformation<'a, 'body, 'alloc, 'tcx> { candidates, // We don't actually store anything at this scope, we just keep things here to be able // to reuse the allocation. - write_info: write_info_alloc, + write_info, // Doesn't matter what we put here, will be overwritten before being used at: Location::START, }; @@ -820,9 +820,9 @@ fn is_local_required(local: Local, body: &Body<'_>) -> bool { ///////////////////////////////////////////////////////// // MIR Dump -fn dest_prop_mir_dump<'body, 'tcx>( +fn dest_prop_mir_dump<'tcx>( tcx: TyCtxt<'tcx>, - body: &'body Body<'tcx>, + body: &Body<'tcx>, points: &DenseLocationMap, live: &SparseIntervalMatrix, round: usize, From 0a282ea7176b000928654262d3faa67785722398 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 29 Aug 2024 11:26:13 +1000 Subject: [PATCH 2/4] Move `WriteInfo` out of `Allocations`. It doesn't need to be in there, and the move simplifies lifetimes. --- compiler/rustc_mir_transform/src/dest_prop.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs index 1022d218f00..f7057b0a6ab 100644 --- a/compiler/rustc_mir_transform/src/dest_prop.rs +++ b/compiler/rustc_mir_transform/src/dest_prop.rs @@ -165,6 +165,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { let def_id = body.source.def_id(); let mut allocations = Allocations::default(); + let mut write_info = WriteInfo::default(); trace!(func = ?tcx.def_path_str(def_id)); let borrowed = rustc_mir_dataflow::impls::borrowed_locals(body); @@ -205,7 +206,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { &mut candidates, &points, &live, - &mut allocations.write_info, + &mut write_info, body, ); @@ -262,7 +263,6 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { struct Allocations { candidates: FxIndexMap>, candidates_reverse: FxIndexMap>, - write_info: WriteInfo, // PERF: Do this for `MaybeLiveLocals` allocations too. } @@ -364,7 +364,7 @@ struct FilterInformation<'a, 'alloc, 'tcx> { points: &'a DenseLocationMap, live: &'a SparseIntervalMatrix, candidates: &'a mut Candidates<'alloc>, - write_info: &'alloc mut WriteInfo, + write_info: &'a mut WriteInfo, at: Location, } @@ -468,7 +468,7 @@ impl<'a, 'alloc, 'tcx> FilterInformation<'a, 'alloc, 'tcx> { candidates: &mut Candidates<'alloc>, points: &DenseLocationMap, live: &SparseIntervalMatrix, - write_info: &'alloc mut WriteInfo, + write_info: &mut WriteInfo, body: &Body<'tcx>, ) { let mut this = FilterInformation { From ad5a6e11c75905a51146f1cd0bb036ebe7c01a29 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 29 Aug 2024 11:30:12 +1000 Subject: [PATCH 3/4] Remove `Allocations`. It's not necessary, and just complicates things. --- compiler/rustc_mir_transform/src/dest_prop.rs | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs index f7057b0a6ab..f5f496acfce 100644 --- a/compiler/rustc_mir_transform/src/dest_prop.rs +++ b/compiler/rustc_mir_transform/src/dest_prop.rs @@ -164,7 +164,8 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { let def_id = body.source.def_id(); - let mut allocations = Allocations::default(); + let mut candidates = FxIndexMap::default(); + let mut candidates_reverse = FxIndexMap::default(); let mut write_info = WriteInfo::default(); trace!(func = ?tcx.def_path_str(def_id)); @@ -196,8 +197,8 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { let mut candidates = find_candidates( body, &borrowed, - &mut allocations.candidates, - &mut allocations.candidates_reverse, + &mut candidates, + &mut candidates_reverse, ); trace!(?candidates); dest_prop_mir_dump(tcx, body, &points, &live, round_count); @@ -255,17 +256,6 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { } } -/// Container for the various allocations that we need. -/// -/// We store these here and hand out `&mut` access to them, instead of dropping and recreating them -/// frequently. Everything with a `&'alloc` lifetime points into here. -#[derive(Default)] -struct Allocations { - candidates: FxIndexMap>, - candidates_reverse: FxIndexMap>, - // PERF: Do this for `MaybeLiveLocals` allocations too. -} - #[derive(Debug)] struct Candidates<'alloc> { /// The set of candidates we are considering in this optimization. From 1be2204363fed9f748860734e7d5f6f7910ecc7e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 29 Aug 2024 11:47:00 +1000 Subject: [PATCH 4/4] Simplify `Candidate`. By making it own the index maps, instead of holding references to them. This requires moving the free function `find_candidate` into `Candidate::reset_and_find`. It lets the `'alloc` lifetime be removed everywhere that still has it. --- compiler/rustc_mir_transform/src/dest_prop.rs | 83 ++++++++----------- 1 file changed, 36 insertions(+), 47 deletions(-) diff --git a/compiler/rustc_mir_transform/src/dest_prop.rs b/compiler/rustc_mir_transform/src/dest_prop.rs index f5f496acfce..ba1e99290fb 100644 --- a/compiler/rustc_mir_transform/src/dest_prop.rs +++ b/compiler/rustc_mir_transform/src/dest_prop.rs @@ -164,8 +164,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) { let def_id = body.source.def_id(); - let mut candidates = FxIndexMap::default(); - let mut candidates_reverse = FxIndexMap::default(); + let mut candidates = Candidates::default(); let mut write_info = WriteInfo::default(); trace!(func = ?tcx.def_path_str(def_id)); @@ -194,12 +193,7 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { loop { // PERF: Can we do something smarter than recalculating the candidates and liveness // results? - let mut candidates = find_candidates( - body, - &borrowed, - &mut candidates, - &mut candidates_reverse, - ); + candidates.reset_and_find(body, &borrowed); trace!(?candidates); dest_prop_mir_dump(tcx, body, &points, &live, round_count); @@ -256,8 +250,8 @@ impl<'tcx> MirPass<'tcx> for DestinationPropagation { } } -#[derive(Debug)] -struct Candidates<'alloc> { +#[derive(Debug, Default)] +struct Candidates { /// The set of candidates we are considering in this optimization. /// /// We will always merge the key into at most one of its values. @@ -272,11 +266,12 @@ struct Candidates<'alloc> { /// /// We will still report that we would like to merge `_1` and `_2` in an attempt to allow us to /// remove that assignment. - c: &'alloc mut FxIndexMap>, + c: FxIndexMap>, + /// A reverse index of the `c` set; if the `c` set contains `a => Place { local: b, proj }`, /// then this contains `b => a`. // PERF: Possibly these should be `SmallVec`s? - reverse: &'alloc mut FxIndexMap>, + reverse: FxIndexMap>, } ////////////////////////////////////////////////////////// @@ -349,11 +344,11 @@ impl<'a, 'tcx> MutVisitor<'tcx> for Merger<'a, 'tcx> { // // This section enforces bullet point 2 -struct FilterInformation<'a, 'alloc, 'tcx> { +struct FilterInformation<'a, 'tcx> { body: &'a Body<'tcx>, points: &'a DenseLocationMap, live: &'a SparseIntervalMatrix, - candidates: &'a mut Candidates<'alloc>, + candidates: &'a mut Candidates, write_info: &'a mut WriteInfo, at: Location, } @@ -361,7 +356,28 @@ struct FilterInformation<'a, 'alloc, 'tcx> { // We first implement some utility functions which we will expose removing candidates according to // different needs. Throughout the liveness filtering, the `candidates` are only ever accessed // through these methods, and not directly. -impl<'alloc> Candidates<'alloc> { +impl Candidates { + /// Collects the candidates for merging. + /// + /// This is responsible for enforcing the first and third bullet point. + fn reset_and_find<'tcx>(&mut self, body: &Body<'tcx>, borrowed: &BitSet) { + self.c.clear(); + self.reverse.clear(); + let mut visitor = FindAssignments { body, candidates: &mut self.c, borrowed }; + visitor.visit_body(body); + // Deduplicate candidates. + for (_, cands) in self.c.iter_mut() { + cands.sort(); + cands.dedup(); + } + // Generate the reverse map. + for (src, cands) in self.c.iter() { + for dest in cands.iter().copied() { + self.reverse.entry(dest).or_default().push(*src); + } + } + } + /// Just `Vec::retain`, but the condition is inverted and we add debugging output fn vec_filter_candidates( src: Local, @@ -436,7 +452,7 @@ enum CandidateFilter { Remove, } -impl<'a, 'alloc, 'tcx> FilterInformation<'a, 'alloc, 'tcx> { +impl<'a, 'tcx> FilterInformation<'a, 'tcx> { /// Filters the set of candidates to remove those that conflict. /// /// The steps we take are exactly those that are outlined at the top of the file. For each @@ -455,7 +471,7 @@ impl<'a, 'alloc, 'tcx> FilterInformation<'a, 'alloc, 'tcx> { /// statement/terminator to be live. We are additionally conservative by treating all written to /// locals as also being read from. fn filter_liveness( - candidates: &mut Candidates<'alloc>, + candidates: &mut Candidates, points: &DenseLocationMap, live: &SparseIntervalMatrix, write_info: &mut WriteInfo, @@ -725,40 +741,13 @@ fn places_to_candidate_pair<'tcx>( Some((a, b)) } -/// Collects the candidates for merging -/// -/// This is responsible for enforcing the first and third bullet point. -fn find_candidates<'alloc, 'tcx>( - body: &Body<'tcx>, - borrowed: &BitSet, - candidates: &'alloc mut FxIndexMap>, - candidates_reverse: &'alloc mut FxIndexMap>, -) -> Candidates<'alloc> { - candidates.clear(); - candidates_reverse.clear(); - let mut visitor = FindAssignments { body, candidates, borrowed }; - visitor.visit_body(body); - // Deduplicate candidates - for (_, cands) in candidates.iter_mut() { - cands.sort(); - cands.dedup(); - } - // Generate the reverse map - for (src, cands) in candidates.iter() { - for dest in cands.iter().copied() { - candidates_reverse.entry(dest).or_default().push(*src); - } - } - Candidates { c: candidates, reverse: candidates_reverse } -} - -struct FindAssignments<'a, 'alloc, 'tcx> { +struct FindAssignments<'a, 'tcx> { body: &'a Body<'tcx>, - candidates: &'alloc mut FxIndexMap>, + candidates: &'a mut FxIndexMap>, borrowed: &'a BitSet, } -impl<'tcx> Visitor<'tcx> for FindAssignments<'_, '_, 'tcx> { +impl<'tcx> Visitor<'tcx> for FindAssignments<'_, 'tcx> { fn visit_statement(&mut self, statement: &Statement<'tcx>, _: Location) { if let StatementKind::Assign(box ( lhs,