From 740f228b5b68f4db74318e0f234786aa3bc4e982 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 21 Apr 2020 10:14:21 -0700 Subject: [PATCH 1/3] Remove `predecessors_for` There is no `Arc::map` equivalent to `LockGuard::map` --- src/librustc_codegen_ssa/mir/mod.rs | 2 +- src/librustc_middle/mir/mod.rs | 13 +------------ .../borrow_check/diagnostics/conflict_errors.rs | 2 +- .../borrow_check/region_infer/values.rs | 2 +- .../borrow_check/type_check/liveness/trace.rs | 2 +- 5 files changed, 5 insertions(+), 16 deletions(-) diff --git a/src/librustc_codegen_ssa/mir/mod.rs b/src/librustc_codegen_ssa/mir/mod.rs index cb6d2d297cd7..1516143cb1ac 100644 --- a/src/librustc_codegen_ssa/mir/mod.rs +++ b/src/librustc_codegen_ssa/mir/mod.rs @@ -155,7 +155,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>( let cleanup_kinds = analyze::cleanup_kinds(&mir); // Allocate a `Block` for every basic block, except // the start block, if nothing loops back to it. - let reentrant_start_block = !mir.predecessors_for(mir::START_BLOCK).is_empty(); + let reentrant_start_block = !mir.predecessors()[mir::START_BLOCK].is_empty(); let block_bxs: IndexVec = mir .basic_blocks() .indices() diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index e476729c11b4..566817ea0d02 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -23,14 +23,12 @@ use rustc_ast::ast::Name; use rustc_data_structures::fx::FxHashSet; use rustc_data_structures::graph::dominators::{dominators, Dominators}; use rustc_data_structures::graph::{self, GraphSuccessors}; -use rustc_data_structures::sync::MappedLockGuard; use rustc_index::bit_set::BitMatrix; use rustc_index::vec::{Idx, IndexVec}; use rustc_macros::HashStable; use rustc_serialize::{Decodable, Encodable}; use rustc_span::symbol::Symbol; use rustc_span::{Span, DUMMY_SP}; -use smallvec::SmallVec; use std::borrow::Cow; use std::fmt::{self, Debug, Display, Formatter, Write}; use std::ops::{Index, IndexMut}; @@ -392,15 +390,6 @@ impl<'tcx> Body<'tcx> { Location { block: bb, statement_index: self[bb].statements.len() } } - #[inline] - pub fn predecessors_for( - &self, - bb: BasicBlock, - ) -> impl std::ops::Deref> + '_ { - let predecessors = self.predecessor_cache.compute(&self.basic_blocks); - MappedLockGuard::map(predecessors, |preds| &mut preds[bb]) - } - #[inline] pub fn predecessors(&self) -> impl std::ops::Deref + '_ { self.predecessor_cache.compute(&self.basic_blocks) @@ -2676,7 +2665,7 @@ impl graph::GraphPredecessors<'graph> for Body<'tcx> { impl graph::WithPredecessors for Body<'tcx> { #[inline] fn predecessors(&self, node: Self::Node) -> >::Iter { - self.predecessors_for(node).clone().into_iter() + self.predecessors()[node].clone().into_iter() } } diff --git a/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs b/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs index 22947610448d..8f771fd738dd 100644 --- a/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs +++ b/src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs @@ -1268,7 +1268,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { location: Location, ) -> impl Iterator + 'a { if location.statement_index == 0 { - let predecessors = body.predecessors_for(location.block).to_vec(); + let predecessors = body.predecessors()[location.block].to_vec(); Either::Left(predecessors.into_iter().map(move |bb| body.terminator_loc(bb))) } else { Either::Right(std::iter::once(Location { diff --git a/src/librustc_mir/borrow_check/region_infer/values.rs b/src/librustc_mir/borrow_check/region_infer/values.rs index 57a3fa6f79b5..6cd814962c61 100644 --- a/src/librustc_mir/borrow_check/region_infer/values.rs +++ b/src/librustc_mir/borrow_check/region_infer/values.rs @@ -89,7 +89,7 @@ impl RegionValueElements { // If this is a basic block head, then the predecessors are // the terminators of other basic blocks stack.extend( - body.predecessors_for(block) + body.predecessors()[block] .iter() .map(|&pred_bb| body.terminator_loc(pred_bb)) .map(|pred_loc| self.point_from_location(pred_loc)), diff --git a/src/librustc_mir/borrow_check/type_check/liveness/trace.rs b/src/librustc_mir/borrow_check/type_check/liveness/trace.rs index af09dc5b8039..ec52a08c7b21 100644 --- a/src/librustc_mir/borrow_check/type_check/liveness/trace.rs +++ b/src/librustc_mir/borrow_check/type_check/liveness/trace.rs @@ -303,7 +303,7 @@ impl LivenessResults<'me, 'typeck, 'flow, 'tcx> { } let body = self.cx.body; - for &pred_block in body.predecessors_for(block).iter() { + for &pred_block in body.predecessors()[block].iter() { debug!("compute_drop_live_points_for_block: pred_block = {:?}", pred_block,); // Check whether the variable is (at least partially) From 59c746030a1f1330f2037f09c2a7a6def8a5fc04 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Tue, 21 Apr 2020 10:15:07 -0700 Subject: [PATCH 2/3] Use a ref-counted pointer for ownership of the predecessor cache ...instead of a `LockGuard` which means the lock is held for longer than necessary. --- src/librustc_middle/mir/predecessors.rs | 37 +++++++++++++++---------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/src/librustc_middle/mir/predecessors.rs b/src/librustc_middle/mir/predecessors.rs index 629b5c2efb71..8f1f92785211 100644 --- a/src/librustc_middle/mir/predecessors.rs +++ b/src/librustc_middle/mir/predecessors.rs @@ -1,5 +1,5 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; -use rustc_data_structures::sync::{Lock, LockGuard, MappedLockGuard}; +use rustc_data_structures::sync::{Lock, Lrc}; use rustc_index::vec::IndexVec; use rustc_serialize as serialize; use smallvec::SmallVec; @@ -11,7 +11,7 @@ pub type Predecessors = IndexVec>; #[derive(Clone, Debug)] pub struct PredecessorCache { - cache: Lock>, + cache: Lock>>, } impl PredecessorCache { @@ -20,30 +20,39 @@ impl PredecessorCache { PredecessorCache { cache: Lock::new(None) } } + /// Invalidates the predecessor cache. + /// + /// Invalidating the predecessor cache requires mutating the MIR, which in turn requires a + /// unique reference (`&mut`) to the `mir::Body`. Because of this, we can assume that all + /// callers of `invalidate` have a unique reference to the MIR and thus to the predecessor + /// cache. This means we don't actually need to take a lock when `invalidate` is called. #[inline] pub fn invalidate(&mut self) { *self.cache.get_mut() = None; } + /// Returns a ref-counted smart pointer containing the predecessor graph for this MIR. + /// + /// We use ref-counting instead of a mapped `LockGuard` here to ensure that the lock for + /// `cache` is only held inside this function. As long as no other locks are taken while + /// computing the predecessor graph, deadlock is impossible. #[inline] pub fn compute( &self, basic_blocks: &IndexVec>, - ) -> MappedLockGuard<'_, Predecessors> { - LockGuard::map(self.cache.lock(), |cache| { - cache.get_or_insert_with(|| { - let mut preds = IndexVec::from_elem(SmallVec::new(), basic_blocks); - for (bb, data) in basic_blocks.iter_enumerated() { - if let Some(term) = &data.terminator { - for &succ in term.successors() { - preds[succ].push(bb); - } + ) -> Lrc { + Lrc::clone(self.cache.lock().get_or_insert_with(|| { + let mut preds = IndexVec::from_elem(SmallVec::new(), basic_blocks); + for (bb, data) in basic_blocks.iter_enumerated() { + if let Some(term) = &data.terminator { + for &succ in term.successors() { + preds[succ].push(bb); } } + } - preds - }) - }) + Lrc::new(preds) + })) } } From 34dfbc3fef3fb02c36beb982523ce8d9d90838e7 Mon Sep 17 00:00:00 2001 From: Dylan MacKenzie Date: Wed, 22 Apr 2020 18:56:23 -0700 Subject: [PATCH 3/3] Add module docs and restrict visibility --- src/librustc_middle/mir/mod.rs | 2 +- src/librustc_middle/mir/predecessors.rs | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/librustc_middle/mir/mod.rs b/src/librustc_middle/mir/mod.rs index 566817ea0d02..858ab0dbe009 100644 --- a/src/librustc_middle/mir/mod.rs +++ b/src/librustc_middle/mir/mod.rs @@ -164,7 +164,7 @@ pub struct Body<'tcx> { /// FIXME(oli-obk): rewrite the promoted during promotion to eliminate the cell components. pub ignore_interior_mut_in_const_validation: bool, - pub predecessor_cache: PredecessorCache, + predecessor_cache: PredecessorCache, } impl<'tcx> Body<'tcx> { diff --git a/src/librustc_middle/mir/predecessors.rs b/src/librustc_middle/mir/predecessors.rs index 8f1f92785211..9508365886aa 100644 --- a/src/librustc_middle/mir/predecessors.rs +++ b/src/librustc_middle/mir/predecessors.rs @@ -1,3 +1,5 @@ +//! Lazily compute the reverse control-flow graph for the MIR. + use rustc_data_structures::stable_hasher::{HashStable, StableHasher}; use rustc_data_structures::sync::{Lock, Lrc}; use rustc_index::vec::IndexVec; @@ -10,13 +12,13 @@ use crate::mir::{BasicBlock, BasicBlockData}; pub type Predecessors = IndexVec>; #[derive(Clone, Debug)] -pub struct PredecessorCache { +pub(super) struct PredecessorCache { cache: Lock>>, } impl PredecessorCache { #[inline] - pub fn new() -> Self { + pub(super) fn new() -> Self { PredecessorCache { cache: Lock::new(None) } } @@ -27,7 +29,7 @@ impl PredecessorCache { /// callers of `invalidate` have a unique reference to the MIR and thus to the predecessor /// cache. This means we don't actually need to take a lock when `invalidate` is called. #[inline] - pub fn invalidate(&mut self) { + pub(super) fn invalidate(&mut self) { *self.cache.get_mut() = None; } @@ -37,7 +39,7 @@ impl PredecessorCache { /// `cache` is only held inside this function. As long as no other locks are taken while /// computing the predecessor graph, deadlock is impossible. #[inline] - pub fn compute( + pub(super) fn compute( &self, basic_blocks: &IndexVec>, ) -> Lrc {