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

Jump to bottom

More MIR borrow check cleanup #56388

Merged
merged 3 commits into from Dec 6, 2018
Merged

More MIR borrow check cleanup #56388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d92287a
Fix some rustc doc links
matthewjasper Nov 30, 2018
4f5b8ea
Remove the `region_map` field from `BorrowSet`
matthewjasper Nov 30, 2018
6000c2e
Use visit_local to find 2PB activations
matthewjasper Nov 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
120 changes: 56 additions & 64 deletions src/librustc_mir/borrow_check/borrow_set.rs
View file
Open in desktop
Expand Up @@ -16,7 +16,7 @@ use rustc::mir::traversal;
use rustc::mir::visit::{
PlaceContext, Visitor, NonUseContext, MutatingUseContext, NonMutatingUseContext
};
use rustc::mir::{self, Location, Mir, Place, Local};
use rustc::mir::{self, Location, Mir, Local};
use rustc::ty::{RegionVid, TyCtxt};
use rustc::util::nodemap::{FxHashMap, FxHashSet};
use rustc_data_structures::indexed_vec::IndexVec;
Expand All @@ -41,10 +41,6 @@ crate struct BorrowSet<'tcx> {
/// only need to store one borrow index
crate activation_map: FxHashMap<Location, Vec<BorrowIndex>>,

/// Every borrow has a region; this maps each such regions back to
/// its borrow-indexes.
crate region_map: FxHashMap<RegionVid, FxHashSet<BorrowIndex>>,

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

Expand Down Expand Up @@ -149,7 +145,6 @@ impl<'tcx> BorrowSet<'tcx> {
idx_vec: IndexVec::new(),
location_map: Default::default(),
activation_map: Default::default(),
region_map: Default::default(),
local_map: Default::default(),
pending_activations: Default::default(),
locals_state_at_exit:
Expand All @@ -164,7 +159,6 @@ impl<'tcx> BorrowSet<'tcx> {
borrows: visitor.idx_vec,
location_map: visitor.location_map,
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 All @@ -184,7 +178,6 @@ struct GatherBorrows<'a, 'gcx: 'tcx, 'tcx: 'a> {
idx_vec: IndexVec<BorrowIndex, BorrowData<'tcx>>,
location_map: FxHashMap<Location, BorrowIndex>,
activation_map: FxHashMap<Location, Vec<BorrowIndex>>,
region_map: FxHashMap<RegionVid, FxHashSet<BorrowIndex>>,
local_map: FxHashMap<mir::Local, FxHashSet<BorrowIndex>>,

/// When we encounter a 2-phase borrow statement, it will always
Expand Down Expand Up @@ -229,7 +222,6 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {

self.insert_as_pending_if_two_phase(location, &assigned_place, kind, idx);

self.region_map.entry(region).or_default().insert(idx);
if let Some(local) = borrowed_place.root_local() {
self.local_map.entry(local).or_default().insert(idx);
}
Expand All @@ -238,68 +230,68 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
self.super_assign(block, assigned_place, rvalue, location)
}

fn visit_place(
fn visit_local(
&mut self,
place: &mir::Place<'tcx>,
temp: &Local,
context: PlaceContext<'tcx>,
location: Location,
) {
self.super_place(place, context, location);

// We found a use of some temporary TEMP...
if let Place::Local(temp) = place {
// ... check whether we (earlier) saw a 2-phase borrow like
//
// TMP = &mut place
if let Some(&borrow_index) = self.pending_activations.get(temp) {
let borrow_data = &mut self.idx_vec[borrow_index];

// Watch out: the use of TMP in the borrow itself
// doesn't count as an activation. =)
if borrow_data.reserve_location == location &&
context == PlaceContext::MutatingUse(MutatingUseContext::Store)
{
return;
}
if !context.is_use() {
return;
}

if let TwoPhaseActivation::ActivatedAt(other_location) =
borrow_data.activation_location {
span_bug!(
self.mir.source_info(location).span,
"found two uses for 2-phase borrow temporary {:?}: \
{:?} and {:?}",
temp,
location,
other_location,
);
}
// We found a use of some temporary TMP
// check whether we (earlier) saw a 2-phase borrow like
//
// TMP = &mut place
if let Some(&borrow_index) = self.pending_activations.get(temp) {
let borrow_data = &mut self.idx_vec[borrow_index];

// Otherwise, this is the unique later use
// that we expect.
borrow_data.activation_location = match context {
// The use of TMP in a shared borrow does not
// count as an actual activation.
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) =>
TwoPhaseActivation::NotActivated,
_ => {
// Double check: This borrow is indeed a two-phase borrow (that is,
// we are 'transitioning' from `NotActivated` to `ActivatedAt`) and
// we've not found any other activations (checked above).
assert_eq!(
borrow_data.activation_location,
TwoPhaseActivation::NotActivated,
"never found an activation for this borrow!",
);

self.activation_map
.entry(location)
.or_default()
.push(borrow_index);
TwoPhaseActivation::ActivatedAt(location)
}
};
// Watch out: the use of TMP in the borrow itself
// doesn't count as an activation. =)
if borrow_data.reserve_location == location &&
context == PlaceContext::MutatingUse(MutatingUseContext::Store)
{
return;
}

if let TwoPhaseActivation::ActivatedAt(other_location) =
borrow_data.activation_location {
span_bug!(
self.mir.source_info(location).span,
"found two uses for 2-phase borrow temporary {:?}: \
{:?} and {:?}",
temp,
location,
other_location,
);
}

// Otherwise, this is the unique later use
// that we expect.
borrow_data.activation_location = match context {
// The use of TMP in a shared borrow does not
// count as an actual activation.
PlaceContext::NonMutatingUse(NonMutatingUseContext::SharedBorrow(..)) |
PlaceContext::NonMutatingUse(NonMutatingUseContext::ShallowBorrow(..)) =>
TwoPhaseActivation::NotActivated,
_ => {
// Double check: This borrow is indeed a two-phase borrow (that is,
// we are 'transitioning' from `NotActivated` to `ActivatedAt`) and
// we've not found any other activations (checked above).
assert_eq!(
borrow_data.activation_location,
TwoPhaseActivation::NotActivated,
"never found an activation for this borrow!",
);

self.activation_map
.entry(location)
.or_default()
.push(borrow_index);
TwoPhaseActivation::ActivatedAt(location)
}
};
}
}

Expand Down
9 changes: 1 addition & 8 deletions src/librustc_mir/dataflow/impls/borrows.rs
View file
Open in desktop
Expand Up @@ -246,7 +246,7 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> {
// re-consider the current implementations of the
// propagate_call_return method.

if let mir::Rvalue::Ref(region, _, ref place) = **rhs {
if let mir::Rvalue::Ref(_, _, ref place) = **rhs {
if place.ignore_borrow(
self.tcx,
self.mir,
Expand All @@ -258,13 +258,6 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> {
panic!("could not find BorrowIndex for location {:?}", location);
});

assert!(self.borrow_set.region_map
.get(&region.to_region_vid())
.unwrap_or_else(|| {
panic!("could not find BorrowIndexs for RegionVid {:?}", region);
})
.contains(&index)
);
sets.gen(*index);

// Issue #46746: Two-phase borrows handles
Expand Down
26 changes: 17 additions & 9 deletions src/librustc_mir/transform/cleanup_post_borrowck.rs
View file
Open in desktop
Expand Up @@ -10,17 +10,25 @@

//! This module provides two passes:
//!
//! - [CleanAscribeUserType], that replaces all
//! [StatementKind::AscribeUserType] statements with [StatementKind::Nop].
//! - [CleanFakeReadsAndBorrows], that replaces all [FakeRead] statements and
//! borrows that are read by [FakeReadCause::ForMatchGuard] fake reads with
//! [StatementKind::Nop].
//! - [`CleanAscribeUserType`], that replaces all [`AscribeUserType`]
//! statements with [`Nop`].
//! - [`CleanFakeReadsAndBorrows`], that replaces all [`FakeRead`] statements
//! and borrows that are read by [`ForMatchGuard`] fake reads with [`Nop`].
//!
//! The [CleanFakeReadsAndBorrows] "pass" is actually implemented as two
//! The `CleanFakeReadsAndBorrows` "pass" is actually implemented as two
//! traversals (aka visits) of the input MIR. The first traversal,
//! [DeleteAndRecordFakeReads], deletes the fake reads and finds the temporaries
//! read by [ForMatchGuard] reads, and [DeleteFakeBorrows] deletes the
//! initialization of those temporaries.
//! [`DeleteAndRecordFakeReads`], deletes the fake reads and finds the
//! temporaries read by [`ForMatchGuard`] reads, and [`DeleteFakeBorrows`]
//! deletes the initialization of those temporaries.
//!
//! [`CleanAscribeUserType`]: cleanup_post_borrowck::CleanAscribeUserType
//! [`CleanFakeReadsAndBorrows`]: cleanup_post_borrowck::CleanFakeReadsAndBorrows
//! [`DeleteAndRecordFakeReads`]: cleanup_post_borrowck::DeleteAndRecordFakeReads
//! [`DeleteFakeBorrows`]: cleanup_post_borrowck::DeleteFakeBorrows
//! [`AscribeUserType`]: rustc::mir::StatementKind::AscribeUserType
//! [`Nop`]: rustc::mir::StatementKind::Nop
//! [`FakeRead`]: rustc::mir::StatementKind::FakeRead
//! [`ForMatchGuard`]: rustc::mir::FakeReadCause::ForMatchGuard

use rustc_data_structures::fx::FxHashSet;

Expand Down