Skip to content

Commit

Permalink
Auto merge of #103947 - camsteffen:place-clones, r=cjgillot
Browse files Browse the repository at this point in the history 
Reduce `PlaceBuilder` cloning

Some API tweaks with an eye towards reducing clones.
  • Loading branch information
@bors
bors committed Nov 23, 2022
2 parents 4e0d0d7 + 9cf6ce0 commit 80b3c6d
Show file tree
Hide file tree
Showing 7 changed files with 134 additions and 150 deletions.
168 changes: 85 additions & 83 deletions compiler/rustc_mir_build/src/build/expr/as_place.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ pub(crate) enum PlaceBase {

/// `PlaceBuilder` is used to create places during MIR construction. It allows you to "build up" a
/// place by pushing more and more projections onto the end, and then convert the final set into a
/// place using the `into_place` method.
/// place using the `to_place` method.
///
/// This is used internally when building a place for an expression like `a.b.c`. The fields `b`
/// and `c` can be progressively pushed onto the place builder that is created when converting `a`.
Expand Down Expand Up @@ -167,59 +167,54 @@ fn find_capture_matching_projections<'a, 'tcx>(
})
}

/// Takes a PlaceBuilder and resolves the upvar (if any) within it, so that the
/// `PlaceBuilder` now starts from `PlaceBase::Local`.
///
/// Returns a Result with the error being the PlaceBuilder (`from_builder`) that was not found.
/// Takes an upvar place and tries to resolve it into a `PlaceBuilder`
/// with `PlaceBase::Local`
#[instrument(level = "trace", skip(cx), ret)]
fn to_upvars_resolved_place_builder<'tcx>(
from_builder: PlaceBuilder<'tcx>,
cx: &Builder<'_, 'tcx>,
) -> Result<PlaceBuilder<'tcx>, PlaceBuilder<'tcx>> {
match from_builder.base {
PlaceBase::Local(_) => Ok(from_builder),
PlaceBase::Upvar { var_hir_id, closure_def_id } => {
let Some((capture_index, capture)) =
find_capture_matching_projections(
&cx.upvars,
var_hir_id,
&from_builder.projection,
) else {
let closure_span = cx.tcx.def_span(closure_def_id);
if !enable_precise_capture(cx.tcx, closure_span) {
bug!(
"No associated capture found for {:?}[{:#?}] even though \
capture_disjoint_fields isn't enabled",
var_hir_id,
from_builder.projection
)
} else {
debug!(
"No associated capture found for {:?}[{:#?}]",
var_hir_id, from_builder.projection,
);
}
return Err(from_builder);
};
var_hir_id: LocalVarId,
closure_def_id: LocalDefId,
projection: &[PlaceElem<'tcx>],
) -> Option<PlaceBuilder<'tcx>> {
let Some((capture_index, capture)) =
find_capture_matching_projections(
&cx.upvars,
var_hir_id,
&projection,
) else {
let closure_span = cx.tcx.def_span(closure_def_id);
if !enable_precise_capture(cx.tcx, closure_span) {
bug!(
"No associated capture found for {:?}[{:#?}] even though \
capture_disjoint_fields isn't enabled",
var_hir_id,
projection
)
} else {
debug!(
"No associated capture found for {:?}[{:#?}]",
var_hir_id, projection,
);
}
return None;
};

// Access the capture by accessing the field within the Closure struct.
let capture_info = &cx.upvars[capture_index];
// Access the capture by accessing the field within the Closure struct.
let capture_info = &cx.upvars[capture_index];

let mut upvar_resolved_place_builder = PlaceBuilder::from(capture_info.use_place);
let mut upvar_resolved_place_builder = PlaceBuilder::from(capture_info.use_place);

// We used some of the projections to build the capture itself,
// now we apply the remaining to the upvar resolved place.
trace!(?capture.captured_place, ?from_builder.projection);
let remaining_projections = strip_prefix(
capture.captured_place.place.base_ty,
from_builder.projection,
&capture.captured_place.place.projections,
);
upvar_resolved_place_builder.projection.extend(remaining_projections);
// We used some of the projections to build the capture itself,
// now we apply the remaining to the upvar resolved place.
trace!(?capture.captured_place, ?projection);
let remaining_projections = strip_prefix(
capture.captured_place.place.base_ty,
projection,
&capture.captured_place.place.projections,
);
upvar_resolved_place_builder.projection.extend(remaining_projections);

Ok(upvar_resolved_place_builder)
}
}
Some(upvar_resolved_place_builder)
}

/// Returns projections remaining after stripping an initial prefix of HIR
Expand All @@ -228,13 +223,14 @@ fn to_upvars_resolved_place_builder<'tcx>(
/// Supports only HIR projection kinds that represent a path that might be
/// captured by a closure or a generator, i.e., an `Index` or a `Subslice`
/// projection kinds are unsupported.
fn strip_prefix<'tcx>(
fn strip_prefix<'a, 'tcx>(
mut base_ty: Ty<'tcx>,
projections: Vec<PlaceElem<'tcx>>,
projections: &'a [PlaceElem<'tcx>],
prefix_projections: &[HirProjection<'tcx>],
) -> impl Iterator<Item = PlaceElem<'tcx>> {
) -> impl Iterator<Item = PlaceElem<'tcx>> + 'a {
let mut iter = projections
.into_iter()
.iter()
.copied()
// Filter out opaque casts, they are unnecessary in the prefix.
.filter(|elem| !matches!(elem, ProjectionElem::OpaqueCast(..)));
for projection in prefix_projections {
Expand All @@ -258,21 +254,21 @@ fn strip_prefix<'tcx>(
}

impl<'tcx> PlaceBuilder<'tcx> {
pub(in crate::build) fn into_place(self, cx: &Builder<'_, 'tcx>) -> Place<'tcx> {
if let PlaceBase::Local(local) = self.base {
Place { local, projection: cx.tcx.intern_place_elems(&self.projection) }
} else {
self.expect_upvars_resolved(cx).into_place(cx)
}
pub(in crate::build) fn to_place(&self, cx: &Builder<'_, 'tcx>) -> Place<'tcx> {
self.try_to_place(cx).unwrap()
}

fn expect_upvars_resolved(self, cx: &Builder<'_, 'tcx>) -> PlaceBuilder<'tcx> {
to_upvars_resolved_place_builder(self, cx).unwrap()
/// Creates a `Place` or returns `None` if an upvar cannot be resolved
pub(in crate::build) fn try_to_place(&self, cx: &Builder<'_, 'tcx>) -> Option<Place<'tcx>> {
let resolved = self.resolve_upvar(cx);
let builder = resolved.as_ref().unwrap_or(self);
let PlaceBase::Local(local) = builder.base else { return None };
let projection = cx.tcx.intern_place_elems(&builder.projection);
Some(Place { local, projection })
}

/// Attempts to resolve the `PlaceBuilder`.
/// On success, it will return the resolved `PlaceBuilder`.
/// On failure, it will return itself.
/// Returns `None` if this is not an upvar.
///
/// Upvars resolve may fail for a `PlaceBuilder` when attempting to
/// resolve a disjoint field whose root variable is not captured
Expand All @@ -281,11 +277,14 @@ impl<'tcx> PlaceBuilder<'tcx> {
/// not captured. This can happen because the final mir that will be
/// generated doesn't require a read for this place. Failures will only
/// happen inside closures.
pub(in crate::build) fn try_upvars_resolved(
self,
pub(in crate::build) fn resolve_upvar(
&self,
cx: &Builder<'_, 'tcx>,
) -> Result<PlaceBuilder<'tcx>, PlaceBuilder<'tcx>> {
to_upvars_resolved_place_builder(self, cx)
) -> Option<PlaceBuilder<'tcx>> {
let PlaceBase::Upvar { var_hir_id, closure_def_id } = self.base else {
return None;
};
to_upvars_resolved_place_builder(cx, var_hir_id, closure_def_id, &self.projection)
}

pub(crate) fn base(&self) -> PlaceBase {
Expand Down Expand Up @@ -316,6 +315,14 @@ impl<'tcx> PlaceBuilder<'tcx> {
self.projection.push(elem);
self
}

/// Same as `.clone().project(..)` but more efficient
pub(crate) fn clone_project(&self, elem: PlaceElem<'tcx>) -> Self {
Self {
base: self.base,
projection: Vec::from_iter(self.projection.iter().copied().chain([elem])),
}
}
}

impl<'tcx> From<Local> for PlaceBuilder<'tcx> {
Expand Down Expand Up @@ -355,7 +362,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expr: &Expr<'tcx>,
) -> BlockAnd<Place<'tcx>> {
let place_builder = unpack!(block = self.as_place_builder(block, expr));
block.and(place_builder.into_place(self))
block.and(place_builder.to_place(self))
}

/// This is used when constructing a compound `Place`, so that we can avoid creating
Expand All @@ -379,7 +386,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expr: &Expr<'tcx>,
) -> BlockAnd<Place<'tcx>> {
let place_builder = unpack!(block = self.as_read_only_place_builder(block, expr));
block.and(place_builder.into_place(self))
block.and(place_builder.to_place(self))
}

/// This is used when constructing a compound `Place`, so that we can avoid creating
Expand Down Expand Up @@ -474,7 +481,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
inferred_ty: expr.ty,
});

let place = place_builder.clone().into_place(this);
let place = place_builder.to_place(this);
this.cfg.push(
block,
Statement {
Expand Down Expand Up @@ -599,22 +606,21 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let is_outermost_index = fake_borrow_temps.is_none();
let fake_borrow_temps = fake_borrow_temps.unwrap_or(base_fake_borrow_temps);

let mut base_place =
let base_place =
unpack!(block = self.expr_as_place(block, base, mutability, Some(fake_borrow_temps),));

// Making this a *fresh* temporary means we do not have to worry about
// the index changing later: Nothing will ever change this temporary.
// The "retagging" transformation (for Stacked Borrows) relies on this.
let idx = unpack!(block = self.as_temp(block, temp_lifetime, index, Mutability::Not,));

block = self.bounds_check(block, base_place.clone(), idx, expr_span, source_info);
block = self.bounds_check(block, &base_place, idx, expr_span, source_info);

if is_outermost_index {
self.read_fake_borrows(block, fake_borrow_temps, source_info)
} else {
base_place = base_place.expect_upvars_resolved(self);
self.add_fake_borrows_of_base(
&base_place,
base_place.to_place(self),
block,
fake_borrow_temps,
expr_span,
Expand All @@ -628,7 +634,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
fn bounds_check(
&mut self,
block: BasicBlock,
slice: PlaceBuilder<'tcx>,
slice: &PlaceBuilder<'tcx>,
index: Local,
expr_span: Span,
source_info: SourceInfo,
Expand All @@ -640,7 +646,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let lt = self.temp(bool_ty, expr_span);

// len = len(slice)
self.cfg.push_assign(block, source_info, len, Rvalue::Len(slice.into_place(self)));
self.cfg.push_assign(block, source_info, len, Rvalue::Len(slice.to_place(self)));
// lt = idx < len
self.cfg.push_assign(
block,
Expand All @@ -658,19 +664,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

fn add_fake_borrows_of_base(
&mut self,
base_place: &PlaceBuilder<'tcx>,
base_place: Place<'tcx>,
block: BasicBlock,
fake_borrow_temps: &mut Vec<Local>,
expr_span: Span,
source_info: SourceInfo,
) {
let tcx = self.tcx;
let local = match base_place.base {
PlaceBase::Local(local) => local,
PlaceBase::Upvar { .. } => bug!("Expected PlacseBase::Local found Upvar"),
};

let place_ty = Place::ty_from(local, &base_place.projection, &self.local_decls, tcx);
let place_ty = base_place.ty(&self.local_decls, tcx);
if let ty::Slice(_) = place_ty.ty.kind() {
// We need to create fake borrows to ensure that the bounds
// check that we just did stays valid. Since we can't assign to
Expand All @@ -680,7 +682,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
match elem {
ProjectionElem::Deref => {
let fake_borrow_deref_ty = Place::ty_from(
local,
base_place.local,
&base_place.projection[..idx],
&self.local_decls,
tcx,
Expand All @@ -698,14 +700,14 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Rvalue::Ref(
tcx.lifetimes.re_erased,
BorrowKind::Shallow,
Place { local, projection },
Place { local: base_place.local, projection },
),
);
fake_borrow_temps.push(fake_borrow_temp);
}
ProjectionElem::Index(_) => {
let index_ty = Place::ty_from(
local,
base_place.local,
&base_place.projection[..idx],
&self.local_decls,
tcx,
Expand Down
7 changes: 3 additions & 4 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let place_builder =
unpack!(block = this.as_place_builder(block, &this.thir[*thir_place]));

if let Ok(place_builder_resolved) = place_builder.try_upvars_resolved(this) {
let mir_place = place_builder_resolved.into_place(this);
if let Some(mir_place) = place_builder.try_to_place(this) {
this.cfg.push_fake_read(
block,
this.source_info(this.tcx.hir().span(*hir_id)),
Expand Down Expand Up @@ -661,7 +660,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
// by the parent itself. The mutability of the current capture
// is same as that of the capture in the parent closure.
PlaceBase::Upvar { .. } => {
let enclosing_upvars_resolved = arg_place_builder.clone().into_place(this);
let enclosing_upvars_resolved = arg_place_builder.to_place(this);

match enclosing_upvars_resolved.as_ref() {
PlaceRef {
Expand Down Expand Up @@ -698,7 +697,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Mutability::Mut => BorrowKind::Mut { allow_two_phase_borrow: false },
};

let arg_place = arg_place_builder.into_place(this);
let arg_place = arg_place_builder.to_place(this);

this.cfg.push_assign(
block,
Expand Down
6 changes: 2 additions & 4 deletions compiler/rustc_mir_build/src/build/expr/into.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
.map(|(n, ty)| match fields_map.get(&n) {
Some(v) => v.clone(),
None => {
let place_builder = place_builder.clone();
this.consume_by_copy_or_move(
place_builder.field(n, *ty).into_place(this),
)
let place = place_builder.clone_project(PlaceElem::Field(n, *ty));
this.consume_by_copy_or_move(place.to_place(this))
}
})
.collect()
Expand Down
Loading

0 comments on commit 80b3c6d

Please sign in to comment.