Skip to content

Commit

Permalink
Auto merge of #113316 - DrMeepster:underefer_perf, r=oli-obk
Browse files Browse the repository at this point in the history 
Rewrite `UnDerefer`, again

This PR is intended to improve the perf regression introduced by #112882.

`UnDerefer` has been separated out again for borrowck reasons. It was a bit overzealous to remove it in the previous PR.

r? `@oli-obk`
  • Loading branch information
@bors
bors committed Jul 11, 2023
2 parents fcaf04e + b0dbd60 commit 5b733e2
Show file tree
Hide file tree
Showing 6 changed files with 186 additions and 124 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_borrowck/src/type_check/liveness/polonius.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ struct UseFactsExtractor<'me, 'tcx> {
}

// A Visitor to walk through the MIR and extract point-wise facts
impl UseFactsExtractor<'_, '_> {
impl<'tcx> UseFactsExtractor<'_, 'tcx> {
fn location_to_index(&self, location: Location) -> LocationIndex {
self.location_table.mid_index(location)
}
Expand All @@ -45,7 +45,7 @@ impl UseFactsExtractor<'_, '_> {
self.path_accessed_at_base.push((path, self.location_to_index(location)));
}

fn place_to_mpi(&self, place: &Place<'_>) -> Option<MovePathIndex> {
fn place_to_mpi(&self, place: &Place<'tcx>) -> Option<MovePathIndex> {
match self.move_data.rev_lookup.find(place.as_ref()) {
LookupResult::Exact(mpi) => Some(mpi),
LookupResult::Parent(mmpi) => mmpi,
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_mir_dataflow/src/impls/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -731,11 +731,11 @@ fn switch_on_enum_discriminant<'mir, 'tcx>(

struct OnMutBorrow<F>(F);

impl<F> Visitor<'_> for OnMutBorrow<F>
impl<'tcx, F> Visitor<'tcx> for OnMutBorrow<F>
where
F: FnMut(&mir::Place<'_>),
F: FnMut(&mir::Place<'tcx>),
{
fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'_>, location: Location) {
fn visit_rvalue(&mut self, rvalue: &mir::Rvalue<'tcx>, location: Location) {
// FIXME: Does `&raw const foo` allow mutation? See #90413.
match rvalue {
mir::Rvalue::Ref(_, mir::BorrowKind::Mut { .. }, place)
Expand All @@ -756,7 +756,7 @@ where
fn for_each_mut_borrow<'tcx>(
mir: &impl MirVisitable<'tcx>,
location: Location,
f: impl FnMut(&mir::Place<'_>),
f: impl FnMut(&mir::Place<'tcx>),
) {
let mut vis = OnMutBorrow(f);

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_mir_dataflow/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ pub mod impls;
pub mod move_paths;
pub mod rustc_peek;
pub mod storage;
pub mod un_derefer;
pub mod value_analysis;

fluent_messages! { "../messages.ftl" }
Expand Down
125 changes: 66 additions & 59 deletions compiler/rustc_mir_dataflow/src/move_paths/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use rustc_middle::mir::*;
use rustc_middle::ty::{self, TyCtxt};
use smallvec::{smallvec, SmallVec};

use std::iter;
use std::mem;

use super::abs_domain::Lift;
Expand Down Expand Up @@ -40,22 +39,22 @@ impl<'a, 'tcx> MoveDataBuilder<'a, 'tcx> {
locals: body
.local_decls
.iter_enumerated()
.filter(|(_, l)| !l.is_deref_temp())
.map(|(i, _)| {
(
i,
.map(|(i, l)| {
if l.is_deref_temp() {
MovePathIndex::MAX
} else {
Self::new_move_path(
&mut move_paths,
&mut path_map,
&mut init_path_map,
None,
Place::from(i),
),
)
)
}
})
.collect(),
projections: Default::default(),
derefer_sidetable: Default::default(),
un_derefer: Default::default(),
},
move_paths,
path_map,
Expand Down Expand Up @@ -100,11 +99,10 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
///
/// Maybe we should have separate "borrowck" and "moveck" modes.
fn move_path_for(&mut self, place: Place<'tcx>) -> Result<MovePathIndex, MoveError<'tcx>> {
let deref_chain = self.builder.data.rev_lookup.deref_chain(place.as_ref());
let data = &mut self.builder.data;

debug!("lookup({:?})", place);
let mut base =
self.builder.data.rev_lookup.find_local(deref_chain.first().unwrap_or(&place).local);
let mut base = data.rev_lookup.find_local(place.local);

// The move path index of the first union that we find. Once this is
// some we stop creating child move paths, since moves from unions
Expand All @@ -113,55 +111,60 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
// from `*(u.f: &_)` isn't allowed.
let mut union_path = None;

for place in deref_chain.into_iter().chain(iter::once(place)) {
for (place_ref, elem) in place.as_ref().iter_projections() {
let body = self.builder.body;
let tcx = self.builder.tcx;
let place_ty = place_ref.ty(body, tcx).ty;
match place_ty.kind() {
ty::Ref(..) | ty::RawPtr(..) => {
return Err(MoveError::cannot_move_out_of(
self.loc,
BorrowedContent {
target_place: place_ref.project_deeper(&[elem], tcx),
},
));
}
ty::Adt(adt, _) if adt.has_dtor(tcx) && !adt.is_box() => {
return Err(MoveError::cannot_move_out_of(
self.loc,
InteriorOfTypeWithDestructor { container_ty: place_ty },
));
}
ty::Adt(adt, _) if adt.is_union() => {
union_path.get_or_insert(base);
}
ty::Slice(_) => {
for (place_ref, elem) in data.rev_lookup.un_derefer.iter_projections(place.as_ref()) {
let body = self.builder.body;
let tcx = self.builder.tcx;
let place_ty = place_ref.ty(body, tcx).ty;
match place_ty.kind() {
ty::Ref(..) | ty::RawPtr(..) => {
return Err(MoveError::cannot_move_out_of(
self.loc,
BorrowedContent { target_place: place_ref.project_deeper(&[elem], tcx) },
));
}
ty::Adt(adt, _) if adt.has_dtor(tcx) && !adt.is_box() => {
return Err(MoveError::cannot_move_out_of(
self.loc,
InteriorOfTypeWithDestructor { container_ty: place_ty },
));
}
ty::Adt(adt, _) if adt.is_union() => {
union_path.get_or_insert(base);
}
ty::Slice(_) => {
return Err(MoveError::cannot_move_out_of(
self.loc,
InteriorOfSliceOrArray {
ty: place_ty,
is_index: matches!(elem, ProjectionElem::Index(..)),
},
));
}

ty::Array(..) => {
if let ProjectionElem::Index(..) = elem {
return Err(MoveError::cannot_move_out_of(
self.loc,
InteriorOfSliceOrArray {
ty: place_ty,
is_index: matches!(elem, ProjectionElem::Index(..)),
},
InteriorOfSliceOrArray { ty: place_ty, is_index: true },
));
}
}

ty::Array(..) => {
if let ProjectionElem::Index(..) = elem {
return Err(MoveError::cannot_move_out_of(
self.loc,
InteriorOfSliceOrArray { ty: place_ty, is_index: true },
));
}
}

_ => {}
};
_ => {}
};

if union_path.is_none() {
base = self
.add_move_path(base, elem, |tcx| place_ref.project_deeper(&[elem], tcx));
}
if union_path.is_none() {
// inlined from add_move_path because of a borrowck conflict with the iterator
base =
*data.rev_lookup.projections.entry((base, elem.lift())).or_insert_with(|| {
MoveDataBuilder::new_move_path(
&mut data.move_paths,
&mut data.path_map,
&mut data.init_path_map,
Some(base),
place_ref.project_deeper(&[elem], tcx),
)
})
}
}

Expand Down Expand Up @@ -282,10 +285,14 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
fn gather_statement(&mut self, stmt: &Statement<'tcx>) {
match &stmt.kind {
StatementKind::Assign(box (place, Rvalue::CopyForDeref(reffed))) => {
assert!(place.projection.is_empty());
if self.builder.body.local_decls[place.local].is_deref_temp() {
self.builder.data.rev_lookup.derefer_sidetable.insert(place.local, *reffed);
}
let local = place.as_local().unwrap();
assert!(self.builder.body.local_decls[local].is_deref_temp());

let rev_lookup = &mut self.builder.data.rev_lookup;

rev_lookup.un_derefer.insert(local, reffed.as_ref());
let base_local = rev_lookup.un_derefer.deref_chain(local).first().unwrap().local;
rev_lookup.locals[local] = rev_lookup.locals[base_local];
}
StatementKind::Assign(box (place, rval)) => {
self.create_move_path(*place);
Expand All @@ -306,7 +313,7 @@ impl<'b, 'a, 'tcx> Gatherer<'b, 'a, 'tcx> {
StatementKind::StorageLive(_) => {}
StatementKind::StorageDead(local) => {
// DerefTemp locals (results of CopyForDeref) don't actually move anything.
if !self.builder.data.rev_lookup.derefer_sidetable.contains_key(&local) {
if !self.builder.body.local_decls[*local].is_deref_temp() {
self.gather_move(Place::from(*local));
}
}
Expand Down
72 changes: 13 additions & 59 deletions compiler/rustc_mir_dataflow/src/move_paths/mod.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::move_paths::builder::MoveDat;
use rustc_data_structures::fx::{FxHashMap, FxIndexMap};
use crate::un_derefer::UnDerefer;
use rustc_data_structures::fx::FxHashMap;
use rustc_index::{IndexSlice, IndexVec};
use rustc_middle::mir::*;
use rustc_middle::ty::{ParamEnv, Ty, TyCtxt};
Expand Down Expand Up @@ -290,7 +291,7 @@ impl Init {
/// Tables mapping from a place to its MovePathIndex.
#[derive(Debug)]
pub struct MovePathLookup<'tcx> {
locals: FxIndexMap<Local, MovePathIndex>,
locals: IndexVec<Local, MovePathIndex>,

/// projections are made from a base-place and a projection
/// elem. The base-place will have a unique MovePathIndex; we use
Expand All @@ -300,8 +301,7 @@ pub struct MovePathLookup<'tcx> {
/// elem to the associated MovePathIndex.
projections: FxHashMap<(MovePathIndex, AbstractElem), MovePathIndex>,

/// Maps `DerefTemp` locals to the `Place`s assigned to them.
derefer_sidetable: FxHashMap<Local, Place<'tcx>>,
un_derefer: UnDerefer<'tcx>,
}

mod builder;
Expand All @@ -317,77 +317,31 @@ impl<'tcx> MovePathLookup<'tcx> {
// alternative will *not* create a MovePath on the fly for an
// unknown place, but will rather return the nearest available
// parent.
pub fn find(&self, place: PlaceRef<'_>) -> LookupResult {
let deref_chain = self.deref_chain(place);
pub fn find(&self, place: PlaceRef<'tcx>) -> LookupResult {
let mut result = self.find_local(place.local);

let local = match deref_chain.first() {
Some(place) => place.local,
None => place.local,
};

let mut result = *self.locals.get(&local).unwrap_or_else(|| {
bug!("base local ({local:?}) of deref_chain should not be a deref temp")
});

// this needs to be a closure because `place` has a different lifetime than `prefix`'s places
let mut subpaths_for_place = |place: PlaceRef<'_>| {
for elem in place.projection.iter() {
if let Some(&subpath) = self.projections.get(&(result, elem.lift())) {
result = subpath;
} else {
return Some(result);
}
}
None
};

for place in deref_chain {
if let Some(result) = subpaths_for_place(place.as_ref()) {
for (_, elem) in self.un_derefer.iter_projections(place) {
if let Some(&subpath) = self.projections.get(&(result, elem.lift())) {
result = subpath;
} else {
return LookupResult::Parent(Some(result));
}
}

if let Some(result) = subpaths_for_place(place) {
return LookupResult::Parent(Some(result));
}

LookupResult::Exact(result)
}

#[inline]
pub fn find_local(&self, local: Local) -> MovePathIndex {
let deref_chain = self.deref_chain(Place::from(local).as_ref());

let local = match deref_chain.last() {
Some(place) => place.local,
None => local,
};

*self.locals.get(&local).unwrap_or_else(|| {
bug!("base local ({local:?}) of deref_chain should not be a deref temp")
})
self.locals[local]
}

/// An enumerated iterator of `local`s and their associated
/// `MovePathIndex`es.
pub fn iter_locals_enumerated(
&self,
) -> impl DoubleEndedIterator<Item = (Local, MovePathIndex)> + ExactSizeIterator + '_ {
self.locals.iter().map(|(&l, &idx)| (l, idx))
}

/// Returns the chain of places behind `DerefTemp` locals in `place`
pub fn deref_chain(&self, place: PlaceRef<'_>) -> Vec<Place<'tcx>> {
let mut prefix = Vec::new();
let mut local = place.local;

while let Some(&reffed) = self.derefer_sidetable.get(&local) {
prefix.insert(0, reffed);
local = reffed.local;
}

debug!("deref_chain({place:?}) = {prefix:?}");

prefix
self.locals.iter_enumerated().map(|(l, &idx)| (l, idx))
}
}

Expand Down

0 comments on commit 5b733e2

Please sign in to comment.