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

Skip a shared borrow of a immutable local variables #53909

Merged
merged 2 commits into from
Sep 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 19 additions & 19 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,6 @@ impl<'tcx> Mir<'tcx> {
} else if self.local_decls[local].name.is_some() {
LocalKind::Var
} else {
debug_assert!(
Copy link
Member

Choose a reason for hiding this comment

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

I'd say just delete the debug_assert!, there's no value in leaving it in commented out

self.local_decls[local].mutability == Mutability::Mut,
"temp should be mutable"
);

LocalKind::Temp
}
}
Expand Down Expand Up @@ -784,33 +779,38 @@ impl<'tcx> LocalDecl<'tcx> {
/// Create a new `LocalDecl` for a temporary.
#[inline]
pub fn new_temp(ty: Ty<'tcx>, span: Span) -> Self {
LocalDecl {
mutability: Mutability::Mut,
ty,
name: None,
source_info: SourceInfo {
span,
scope: OUTERMOST_SOURCE_SCOPE,
},
visibility_scope: OUTERMOST_SOURCE_SCOPE,
internal: false,
is_user_variable: None,
}
Self::new_local(ty, Mutability::Mut, false, span)
}

/// Create a new immutable `LocalDecl` for a temporary.
#[inline]
pub fn new_immutable_temp(ty: Ty<'tcx>, span: Span) -> Self {
Self::new_local(ty, Mutability::Not, false, span)
}

/// Create a new `LocalDecl` for a internal temporary.
#[inline]
pub fn new_internal(ty: Ty<'tcx>, span: Span) -> Self {
Self::new_local(ty, Mutability::Mut, true, span)
}

#[inline]
fn new_local(
ty: Ty<'tcx>,
mutability: Mutability,
internal: bool,
span: Span,
) -> Self {
LocalDecl {
mutability: Mutability::Mut,
mutability,
ty,
name: None,
source_info: SourceInfo {
span,
scope: OUTERMOST_SOURCE_SCOPE,
},
visibility_scope: OUTERMOST_SOURCE_SCOPE,
internal: true,
internal,
is_user_variable: None,
}
}
Expand Down
60 changes: 57 additions & 3 deletions src/librustc_mir/borrow_check/borrow_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,14 @@

use borrow_check::place_ext::PlaceExt;
use dataflow::indexes::BorrowIndex;
use dataflow::move_paths::MoveData;
use rustc::mir::traversal;
use rustc::mir::visit::{PlaceContext, Visitor};
use rustc::mir::{self, Location, Mir, Place};
use rustc::mir::{self, Location, Mir, Place, Local};
use rustc::ty::{Region, TyCtxt};
use rustc::util::nodemap::{FxHashMap, FxHashSet};
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_data_structures::bitvec::BitArray;
use std::fmt;
use std::hash::Hash;
use std::ops::Index;
Expand Down Expand Up @@ -43,6 +45,8 @@ crate struct BorrowSet<'tcx> {

/// Map from local to all the borrows on that local
crate local_map: FxHashMap<mir::Local, FxHashSet<BorrowIndex>>,

crate locals_state_at_exit: LocalsStateAtExit,
}

impl<'tcx> Index<BorrowIndex> for BorrowSet<'tcx> {
Expand Down Expand Up @@ -96,8 +100,52 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> {
}
}

crate enum LocalsStateAtExit {
AllAreInvalidated,
SomeAreInvalidated { has_storage_dead_or_moved: BitArray<Local> }
}

impl LocalsStateAtExit {
fn build(
locals_are_invalidated_at_exit: bool,
mir: &Mir<'tcx>,
move_data: &MoveData<'tcx>
) -> Self {
struct HasStorageDead(BitArray<Local>);

impl<'tcx> Visitor<'tcx> for HasStorageDead {
fn visit_local(&mut self, local: &Local, ctx: PlaceContext<'tcx>, _: Location) {
if ctx == PlaceContext::StorageDead {
self.0.insert(*local);
}
}
}

if locals_are_invalidated_at_exit {
LocalsStateAtExit::AllAreInvalidated
} else {
let mut has_storage_dead = HasStorageDead(BitArray::new(mir.local_decls.len()));
has_storage_dead.visit_mir(mir);
let mut has_storage_dead_or_moved = has_storage_dead.0;
for move_out in &move_data.moves {
if let Some(index) = move_data.base_local(move_out.path) {
has_storage_dead_or_moved.insert(index);

}
}
LocalsStateAtExit::SomeAreInvalidated{ has_storage_dead_or_moved }
}
}
}

impl<'tcx> BorrowSet<'tcx> {
pub fn build(tcx: TyCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) -> Self {
pub fn build(
tcx: TyCtxt<'_, '_, 'tcx>,
mir: &Mir<'tcx>,
locals_are_invalidated_at_exit: bool,
move_data: &MoveData<'tcx>
) -> Self {

let mut visitor = GatherBorrows {
tcx,
mir,
Expand All @@ -107,6 +155,8 @@ impl<'tcx> BorrowSet<'tcx> {
region_map: FxHashMap(),
local_map: FxHashMap(),
pending_activations: FxHashMap(),
locals_state_at_exit:
LocalsStateAtExit::build(locals_are_invalidated_at_exit, mir, move_data),
};

for (block, block_data) in traversal::preorder(mir) {
Expand All @@ -119,6 +169,7 @@ impl<'tcx> BorrowSet<'tcx> {
activation_map: visitor.activation_map,
region_map: visitor.region_map,
local_map: visitor.local_map,
locals_state_at_exit: visitor.locals_state_at_exit,
}
}

Expand Down Expand Up @@ -148,6 +199,8 @@ struct GatherBorrows<'a, 'gcx: 'tcx, 'tcx: 'a> {
/// the borrow. When we find a later use of this activation, we
/// remove from the map (and add to the "tombstone" set below).
pending_activations: FxHashMap<mir::Local, BorrowIndex>,

locals_state_at_exit: LocalsStateAtExit,
}

impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
Expand All @@ -159,7 +212,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
location: mir::Location,
) {
if let mir::Rvalue::Ref(region, kind, ref borrowed_place) = *rvalue {
if borrowed_place.ignore_borrow(self.tcx, self.mir) {
if borrowed_place.ignore_borrow(
self.tcx, self.mir, &self.locals_state_at_exit) {
return;
}

Expand Down
12 changes: 7 additions & 5 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,12 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
|bd, i| DebugFormatted::new(&bd.move_data().inits[i]),
));

let borrow_set = Rc::new(BorrowSet::build(tcx, mir));
let locals_are_invalidated_at_exit = match tcx.hir.body_owner_kind(id) {
hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => false,
hir::BodyOwnerKind::Fn => true,
};
let borrow_set = Rc::new(BorrowSet::build(
tcx, mir, locals_are_invalidated_at_exit, &mdpe.move_data));

// If we are in non-lexical mode, compute the non-lexical lifetimes.
let (regioncx, polonius_output, opt_closure_req) = nll::compute_regions(
Expand Down Expand Up @@ -241,10 +246,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
param_env: param_env,
location_table,
movable_generator,
locals_are_invalidated_at_exit: match tcx.hir.body_owner_kind(id) {
hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => false,
hir::BodyOwnerKind::Fn => true,
},
locals_are_invalidated_at_exit,
access_place_error_reported: FxHashSet(),
reservation_error_reported: FxHashSet(),
moved_error_reported: FxHashSet(),
Expand Down
45 changes: 38 additions & 7 deletions src/librustc_mir/borrow_check/place_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,57 @@

use rustc::hir;
use rustc::mir::ProjectionElem;
use rustc::mir::{Local, Mir, Place};
use rustc::mir::{Local, Mir, Place, Mutability};
use rustc::ty::{self, TyCtxt};
use borrow_check::borrow_set::LocalsStateAtExit;

/// Extension methods for the `Place` type.
crate trait PlaceExt<'tcx> {
/// Returns true if we can safely ignore borrows of this place.
/// This is true whenever there is no action that the user can do
/// to the place `self` that would invalidate the borrow. This is true
/// for borrows of raw pointer dereferents as well as shared references.
fn ignore_borrow(&self, tcx: TyCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) -> bool;
fn ignore_borrow(
&self,
tcx: TyCtxt<'_, '_, 'tcx>,
mir: &Mir<'tcx>,
locals_state_at_exit: &LocalsStateAtExit,
) -> bool;

/// If this is a place like `x.f.g`, returns the local
/// `x`. Returns `None` if this is based in a static.
fn root_local(&self) -> Option<Local>;
}

impl<'tcx> PlaceExt<'tcx> for Place<'tcx> {
fn ignore_borrow(&self, tcx: TyCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) -> bool {
fn ignore_borrow(
&self,
tcx: TyCtxt<'_, '_, 'tcx>,
mir: &Mir<'tcx>,
locals_state_at_exit: &LocalsStateAtExit,
) -> bool {
match self {
Place::Promoted(_) |
Place::Local(_) => false,
Place::Promoted(_) => false,

// If a local variable is immutable, then we only need to track borrows to guard
// against two kinds of errors:
// * The variable being dropped while still borrowed (e.g., because the fn returns
// a reference to a local variable)
// * The variable being moved while still borrowed
//
// In particular, the variable cannot be mutated -- the "access checks" will fail --
// so we don't have to worry about mutation while borrowed.
Place::Local(index) => {
match locals_state_at_exit {
LocalsStateAtExit::AllAreInvalidated => false,
LocalsStateAtExit::SomeAreInvalidated { has_storage_dead_or_moved } => {
let ignore = !has_storage_dead_or_moved.contains(*index) &&
mir.local_decls[*index].mutability == Mutability::Not;
debug!("ignore_borrow: local {:?} => {:?}", index, ignore);
ignore
}
}
}
Place::Static(static_) => {
tcx.is_static(static_.def_id) == Some(hir::Mutability::MutMutable)
}
Expand All @@ -39,7 +69,8 @@ impl<'tcx> PlaceExt<'tcx> for Place<'tcx> {
| ProjectionElem::Downcast(..)
| ProjectionElem::Subslice { .. }
| ProjectionElem::ConstantIndex { .. }
| ProjectionElem::Index(_) => proj.base.ignore_borrow(tcx, mir),
| ProjectionElem::Index(_) => proj.base.ignore_borrow(
tcx, mir, locals_state_at_exit),

ProjectionElem::Deref => {
let ty = proj.base.ty(mir, tcx).to_ty(tcx);
Expand All @@ -55,7 +86,7 @@ impl<'tcx> PlaceExt<'tcx> for Place<'tcx> {
// borrowed *that* one, leaving the original
// path unborrowed.
ty::RawPtr(..) | ty::Ref(_, _, hir::MutImmutable) => true,
_ => proj.base.ignore_borrow(tcx, mir),
_ => proj.base.ignore_borrow(tcx, mir, locals_state_at_exit),
}
}
},
Expand Down
32 changes: 20 additions & 12 deletions src/librustc_mir/build/expr/as_constant.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,26 +18,34 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
/// Compile `expr`, yielding a compile-time constant. Assumes that
/// `expr` is a valid compile-time constant!
pub fn as_constant<M>(&mut self, expr: M) -> Constant<'tcx>
where M: Mirror<'tcx, Output=Expr<'tcx>>
where
M: Mirror<'tcx, Output = Expr<'tcx>>,
{
let expr = self.hir.mirror(expr);
self.expr_as_constant(expr)
}

fn expr_as_constant(&mut self, expr: Expr<'tcx>) -> Constant<'tcx> {
let this = self;
let Expr { ty, temp_lifetime: _, span, kind }
= expr;
let Expr {
ty,
temp_lifetime: _,
span,
kind,
} = expr;
match kind {
ExprKind::Scope { region_scope: _, lint_level: _, value } =>
this.as_constant(value),
ExprKind::Literal { literal, user_ty } =>
Constant { span, ty, user_ty, literal },
_ =>
span_bug!(
span,
"expression is not a valid constant {:?}",
kind),
ExprKind::Scope {
region_scope: _,
lint_level: _,
value,
} => this.as_constant(value),
ExprKind::Literal { literal, user_ty } => Constant {
span,
ty,
user_ty,
literal,
},
_ => span_bug!(span, "expression is not a valid constant {:?}", kind),
}
}
}
Loading