Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove interior mutability in mir predecessors cache #64736

Merged
merged 47 commits into from
Dec 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
c16ef6b
Remove interior mutability in mir predecessors cache
Nashenas88 Sep 24, 2019
b524059
Inline cache impl into Body, rename predecessor fns, change output of…
Nashenas88 Sep 25, 2019
ce29f43
Get rid of old comment
Nashenas88 Sep 25, 2019
f534d9f
Stop invalidating predecessors cache when accessing unique basic bloc…
Nashenas88 Sep 27, 2019
2b31456
Add pass to ensure predecessors cache is generated after optimization
Nashenas88 Sep 28, 2019
e1afa51
Fix Mir visitor macro to ensure it calls the proper method to invalid…
Nashenas88 Sep 28, 2019
570e418
Address linting errors caught by CI
Nashenas88 Sep 28, 2019
8e8c97e
Ensure predecessors are recomputed at critical points, fixes panics
Nashenas88 Oct 3, 2019
ad73468
Move predecessors cache invalidation back to basic_blocks_mut, add a …
Nashenas88 Oct 4, 2019
94414ac
Address excessive line length that was triggering warning during linting
Nashenas88 Oct 4, 2019
52cc85f
Address nits and remove unneeded pass
Nashenas88 Oct 4, 2019
22bc8a0
Add back cache invalidation to basic_blocks_and_local_decls_mut
Nashenas88 Oct 4, 2019
9b335ce
Move predecessors cache back to its own type
Nashenas88 Oct 7, 2019
c0592fa
Move predecessor cache outside of Body, use wrapper types to manage C…
Nashenas88 Oct 10, 2019
649c73f
Simplify Cache wrapper to single type, impl Deref on it, fix all comp…
Nashenas88 Oct 11, 2019
30b1d9e
Remove Body from FunctionCx, pass it along during librustc_codegen_ssa
Nashenas88 Oct 12, 2019
3d68f5f
Improved BodyCache body impl so it only returns a sharable ref, add n…
Nashenas88 Oct 14, 2019
16952cc
Add Body back as field of FunctionCx, but under a different lifetime
Nashenas88 Oct 14, 2019
66279d1
Revert back to using FunctionCx's Body
Nashenas88 Oct 14, 2019
c8c266a
Convert &mut to & since the reference didn't need to be mutable
Nashenas88 Oct 14, 2019
2eed90a
Account for new maybe_sideeffect helper that requires predecessors
Nashenas88 Oct 23, 2019
ab98c59
Fix a large number of Body -> (ReadOnly)BodyCache type errors, add pr…
Nashenas88 Oct 24, 2019
26f1c01
Add read_only fn to BodyCache<&mut...> impl, fix more Body -> (ReadOn…
Nashenas88 Oct 24, 2019
0a19371
Add predecessors fn to ReadOnlyBodyCache, fix more Body -> (ReadOnly)…
Nashenas88 Oct 25, 2019
3642a71
Fix typo caused by rebasing
Nashenas88 Oct 25, 2019
38c0887
Fix remaining Body -> (ReadOnly)BodyCache type errors in librustc_mir…
Nashenas88 Oct 25, 2019
fc6b58d
Simplify BodyCache impl and fix all remaining type errors in librustc…
Nashenas88 Oct 26, 2019
4de31b2
Fix remaining compilation issues
Nashenas88 Oct 28, 2019
35590b5
Fix typos caused during rebase
Nashenas88 Oct 28, 2019
67b7a78
Fix tidy errors
Nashenas88 Oct 28, 2019
ab657e3
Fix typo
Nashenas88 Oct 28, 2019
e54c610
Fix compilation errors created during rebase
Nashenas88 Nov 4, 2019
c42bdb8
Undo minor changes that weren't needed, fix one lifetime typo
Nashenas88 Nov 5, 2019
595d161
Remove BodyCache.body and rely on Deref as much as possible for ReadO…
Nashenas88 Nov 6, 2019
b2fe254
Remove HasLocalDecls impl from BodyCache's, properly reborrow to Body…
Nashenas88 Nov 6, 2019
51b0665
Fix typos caused during rebase
Nashenas88 Nov 6, 2019
ed90818
Remove files created during conflict resolution
Nashenas88 Nov 6, 2019
05dc5e9
Compute predecessors in mir_build query and use existing cache for ge…
Nashenas88 Nov 13, 2019
245abc4
Fix type errors cause during rebasing
Nashenas88 Nov 13, 2019
c6354e9
Remove inline attributes that hadn't been profiled, unexport Cache si…
Nashenas88 Nov 20, 2019
598797c
Remove unchecked inline attribute, remove unused functions, make chac…
Nashenas88 Nov 20, 2019
64654ce
Fix type errors created during rebasing
Nashenas88 Nov 21, 2019
9978574
Fix rebasing errors, convert some BodyCache::body() calls to reborrows
Nashenas88 Nov 25, 2019
acb90eb
Fix tidy issues
Nashenas88 Nov 25, 2019
38bd3a2
Use new HashStable proc macro
Nashenas88 Nov 27, 2019
6123478
Fix issues caused during rebasing
Nashenas88 Nov 29, 2019
3eaad56
Fix issues caused during rebasing
Nashenas88 Dec 2, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/librustc/arena.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,17 @@ macro_rules! arena_types {
[] generics: rustc::ty::Generics,
[] trait_def: rustc::ty::TraitDef,
[] adt_def: rustc::ty::AdtDef,
[] steal_mir: rustc::ty::steal::Steal<rustc::mir::Body<$tcx>>,
[] mir: rustc::mir::Body<$tcx>,
[] steal_mir: rustc::ty::steal::Steal<rustc::mir::BodyCache<$tcx>>,
[] mir: rustc::mir::BodyCache<$tcx>,
[] steal_promoted: rustc::ty::steal::Steal<
rustc_index::vec::IndexVec<
rustc::mir::Promoted,
rustc::mir::Body<$tcx>
rustc::mir::BodyCache<$tcx>
>
>,
[] promoted: rustc_index::vec::IndexVec<
rustc::mir::Promoted,
rustc::mir::Body<$tcx>
rustc::mir::BodyCache<$tcx>
>,
[] tables: rustc::ty::TypeckTables<$tcx>,
[] const_allocs: rustc::mir::interpret::Allocation,
Expand Down
274 changes: 251 additions & 23 deletions src/librustc/mir/cache.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,19 @@
use rustc_index::vec::IndexVec;
use rustc_data_structures::sync::{RwLock, MappedReadGuard, ReadGuard};
use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
use rustc_serialize::{Encodable, Encoder, Decodable, Decoder};
use crate::ich::StableHashingContext;
use crate::mir::{Body, BasicBlock};
use crate::mir::{BasicBlock, BasicBlockData, Body, LocalDecls, Location, Successors};
use rustc_data_structures::graph::{self, GraphPredecessors, GraphSuccessors};
use rustc_data_structures::graph::dominators::{dominators, Dominators};
use std::iter;
use std::ops::{Deref, DerefMut, Index, IndexMut};
use std::vec::IntoIter;

#[derive(Clone, Debug)]
pub struct Cache {
predecessors: RwLock<Option<IndexVec<BasicBlock, Vec<BasicBlock>>>>
predecessors: Option<IndexVec<BasicBlock, Vec<BasicBlock>>>,
}


impl rustc_serialize::Encodable for Cache {
fn encode<S: Encoder>(&self, s: &mut S) -> Result<(), S::Error> {
Encodable::encode(&(), s)
Expand All @@ -31,39 +34,264 @@ impl<'a> HashStable<StableHashingContext<'a>> for Cache {

impl Cache {
pub fn new() -> Self {
Cache {
predecessors: RwLock::new(None)
Self {
predecessors: None,
}
}

pub fn invalidate(&self) {
pub fn invalidate_predecessors(&mut self) {
// FIXME: consider being more fine-grained
*self.predecessors.borrow_mut() = None;
self.predecessors = None;
}

pub fn predecessors(
&self,
body: &Body<'_>
) -> MappedReadGuard<'_, IndexVec<BasicBlock, Vec<BasicBlock>>> {
if self.predecessors.borrow().is_none() {
*self.predecessors.borrow_mut() = Some(calculate_predecessors(body));
pub fn ensure_predecessors(&mut self, body: &Body<'_>) {
if self.predecessors.is_none() {
let mut result = IndexVec::from_elem(vec![], body.basic_blocks());
for (bb, data) in body.basic_blocks().iter_enumerated() {
if let Some(ref term) = data.terminator {
for &tgt in term.successors() {
result[tgt].push(bb);
}
}
}

self.predecessors = Some(result)
}
}

/// This will recompute the predecessors cache if it is not available
fn predecessors(&mut self, body: &Body<'_>) -> &IndexVec<BasicBlock, Vec<BasicBlock>> {
self.ensure_predecessors(body);
self.predecessors.as_ref().unwrap()
}

fn unwrap_predecessors_for(&self, bb: BasicBlock) -> &[BasicBlock] {
&self.predecessors.as_ref().unwrap()[bb]
}

ReadGuard::map(self.predecessors.borrow(), |p| p.as_ref().unwrap())
fn unwrap_predecessor_locations<'a>(
&'a self,
loc: Location,
body: &'a Body<'a>
) -> impl Iterator<Item = Location> + 'a {
let if_zero_locations = if loc.statement_index == 0 {
let predecessor_blocks = self.unwrap_predecessors_for(loc.block);
let num_predecessor_blocks = predecessor_blocks.len();
Some(
(0..num_predecessor_blocks)
.map(move |i| predecessor_blocks[i])
.map(move |bb| body.terminator_loc(bb)),
)
} else {
None
};

let if_not_zero_locations = if loc.statement_index == 0 {
None
} else {
Some(Location { block: loc.block, statement_index: loc.statement_index - 1 })
};

if_zero_locations.into_iter().flatten().chain(if_not_zero_locations)
}

pub fn basic_blocks_mut<'a, 'tcx>(
&mut self,
body: &'a mut Body<'tcx>
) -> &'a mut IndexVec<BasicBlock, BasicBlockData<'tcx>> {
debug!("bbm: Clearing predecessors cache for body at: {:?}", body.span.data());
self.invalidate_predecessors();
&mut body.basic_blocks
}

pub fn basic_blocks_and_local_decls_mut<'a, 'tcx>(
&mut self,
body: &'a mut Body<'tcx>
) -> (&'a mut IndexVec<BasicBlock, BasicBlockData<'tcx>>, &'a mut LocalDecls<'tcx>) {
debug!("bbaldm: Clearing predecessors cache for body at: {:?}", body.span.data());
self.invalidate_predecessors();
(&mut body.basic_blocks, &mut body.local_decls)
}
}

fn calculate_predecessors(body: &Body<'_>) -> IndexVec<BasicBlock, Vec<BasicBlock>> {
let mut result = IndexVec::from_elem(vec![], body.basic_blocks());
for (bb, data) in body.basic_blocks().iter_enumerated() {
if let Some(ref term) = data.terminator {
for &tgt in term.successors() {
result[tgt].push(bb);
}
#[derive(Clone, Debug, HashStable, RustcEncodable, RustcDecodable, TypeFoldable)]
pub struct BodyCache<'tcx> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be named BodyAndCache to alleviate confusion.

cache: Cache,
body: Body<'tcx>,
}

impl BodyCache<'tcx> {
pub fn new(body: Body<'tcx>) -> Self {
Self {
cache: Cache::new(),
body,
}
}
}

#[macro_export]
macro_rules! read_only {
($body:expr) => {
{
$body.ensure_predecessors();
$body.unwrap_read_only()
}
};
}

impl BodyCache<'tcx> {
pub fn ensure_predecessors(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to take self by mut ref.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does. Cache's ensure_predecessors takes self by mut ref (https://github.com/rust-lang/rust/pull/64736/files/3eaad564d25402013bdf5591c453c916b49cdf93#diff-f1f20e32c0d3522f2f33135df0f4696bR47), and that requires that self.cache here below is taken by mut ref. That's only possible if you have &mut self. Or is there some detail I'm overlooking?

self.cache.ensure_predecessors(&self.body);
}

pub fn predecessors(&mut self) -> &IndexVec<BasicBlock, Vec<BasicBlock>> {
self.cache.predecessors(&self.body)
}

pub fn unwrap_read_only(&self) -> ReadOnlyBodyCache<'_, 'tcx> {
ReadOnlyBodyCache::new(&self.cache, &self.body)
}

pub fn basic_blocks_mut(&mut self) -> &mut IndexVec<BasicBlock, BasicBlockData<'tcx>> {
self.cache.basic_blocks_mut(&mut self.body)
}

pub fn basic_blocks_and_local_decls_mut(
&mut self
) -> (&mut IndexVec<BasicBlock, BasicBlockData<'tcx>>, &mut LocalDecls<'tcx>) {
self.cache.basic_blocks_and_local_decls_mut(&mut self.body)
}
}

impl<'tcx> Index<BasicBlock> for BodyCache<'tcx> {
type Output = BasicBlockData<'tcx>;

fn index(&self, index: BasicBlock) -> &BasicBlockData<'tcx> {
&self.body[index]
}
}

impl<'tcx> IndexMut<BasicBlock> for BodyCache<'tcx> {
fn index_mut(&mut self, index: BasicBlock) -> &mut Self::Output {
&mut self.basic_blocks_mut()[index]
}
}

impl<'tcx> Deref for BodyCache<'tcx> {
type Target = Body<'tcx>;

fn deref(&self) -> &Self::Target {
&self.body
}
}

impl<'tcx> DerefMut for BodyCache<'tcx> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.body
}
}

#[derive(Copy, Clone, Debug)]
pub struct ReadOnlyBodyCache<'a, 'tcx> {
cache: &'a Cache,
body: &'a Body<'tcx>,
}

impl ReadOnlyBodyCache<'a, 'tcx> {
fn new(cache: &'a Cache, body: &'a Body<'tcx>) -> Self {
assert!(
cache.predecessors.is_some(),
"Cannot construct ReadOnlyBodyCache without computed predecessors");
Self {
cache,
body,
}
}

result
pub fn predecessors(&self) -> &IndexVec<BasicBlock, Vec<BasicBlock>> {
self.cache.predecessors.as_ref().unwrap()
}

pub fn predecessors_for(&self, bb: BasicBlock) -> &[BasicBlock] {
self.cache.unwrap_predecessors_for(bb)
}

pub fn predecessor_locations(&self, loc: Location) -> impl Iterator<Item = Location> + '_ {
self.cache.unwrap_predecessor_locations(loc, self.body)
}

pub fn body(&self) -> &'a Body<'tcx> {
self.body
}

pub fn basic_blocks(&self) -> &IndexVec<BasicBlock, BasicBlockData<'tcx>> {
&self.body.basic_blocks
}

pub fn dominators(&self) -> Dominators<BasicBlock> {
dominators(self)
}
}

impl graph::DirectedGraph for ReadOnlyBodyCache<'a, 'tcx> {
type Node = BasicBlock;
}

impl graph::GraphPredecessors<'graph> for ReadOnlyBodyCache<'a, 'tcx> {
type Item = BasicBlock;
type Iter = IntoIter<BasicBlock>;
}

impl graph::WithPredecessors for ReadOnlyBodyCache<'a, 'tcx> {
fn predecessors(
&self,
node: Self::Node,
) -> <Self as GraphPredecessors<'_>>::Iter {
self.cache.unwrap_predecessors_for(node).to_vec().into_iter()
}
}

impl graph::WithNumNodes for ReadOnlyBodyCache<'a, 'tcx> {
fn num_nodes(&self) -> usize {
self.body.num_nodes()
}
}

impl graph::WithStartNode for ReadOnlyBodyCache<'a, 'tcx> {
fn start_node(&self) -> Self::Node {
self.body.start_node()
}
}

impl graph::WithSuccessors for ReadOnlyBodyCache<'a, 'tcx> {
fn successors(
&self,
node: Self::Node,
) -> <Self as GraphSuccessors<'_>>::Iter {
self.body.successors(node)
}
}

impl<'a, 'b, 'tcx> graph::GraphSuccessors<'b> for ReadOnlyBodyCache<'a, 'tcx> {
type Item = BasicBlock;
type Iter = iter::Cloned<Successors<'b>>;
}


impl Deref for ReadOnlyBodyCache<'a, 'tcx> {
type Target = Body<'tcx>;
Copy link
Member

@eddyb eddyb Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, this is a common mistake for wrappers of references.

The target should be the reference, &'a Body<'tcx>, so that you can actually get e.g. &'tcx mir::BasicBlockData<'tcx> when 'a = 'tcx.

I wonder if this PR added any lifetimes to existing code to work around that (likely not codegen because that was already wrongly using &'a Body<'tcx> instead of &'tcx Body<'tcx>).

EDIT: thankfully doesn't seem like it. I will likely fix the Deref impl in #65947.


fn deref(&self) -> &Self::Target {
self.body
}
}

impl Index<BasicBlock> for ReadOnlyBodyCache<'a, 'tcx> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need this, since you have Deref.

type Output = BasicBlockData<'tcx>;

fn index(&self, index: BasicBlock) -> &BasicBlockData<'tcx> {
&self.body[index]
}
}

CloneTypeFoldableAndLiftImpls! {
Expand Down
Loading