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

[nll] change how MIR represents places #53247

Closed
wants to merge 33 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
357bc2c
Introduce new Place in librustc/mir
csmoe Aug 8, 2018
34329c0
Intern Place elems
csmoe Aug 8, 2018
bd9f156
Add split_projection/base_place method
csmoe Aug 8, 2018
85a87a1
Try read Place
csmoe Aug 8, 2018
a5c428b
Replace with new Place in mir/borrowck
csmoe Aug 9, 2018
6002649
Clean up old Place in rustc_mir
csmoe Aug 10, 2018
b1dbe6f
Intern PlaceElem with iterator
csmoe Aug 17, 2018
c5a7e57
Remove Projection and impl stablehash for Place with macro
csmoe Aug 17, 2018
e684ea3
Rewrite places_conflict
csmoe Aug 19, 2018
091802d
Rewrite recurseive place traversal to iteration
csmoe Aug 19, 2018
c1fff48
Port new Place into llvm codegen
csmoe Aug 22, 2018
56773a9
refactor `PlaceTy::base_ty` into a method on `PlaceBase`
nikomatsakis Aug 24, 2018
36b5cac
fix the whitespace after `elems}`
nikomatsakis Aug 24, 2018
2cd7680
rewrite `builder` so that it terminates
nikomatsakis Aug 24, 2018
ec40255
Rewrite place workflow in rustc_mir/interpret
csmoe Aug 25, 2018
7b65566
Rewrite place workflow in rustc_mir/borrow_check
csmoe Aug 28, 2018
a6520ec
Rewrite place workflow in rustc_mir/dataflow
csmoe Aug 28, 2018
fea2e88
Rewrite place workflow in rustc_mir/transform
csmoe Aug 28, 2018
244fbb4
Rewrite place workflow in rustc_codegen_llvm
csmoe Aug 28, 2018
73e86a2
Rewrite place workflow in rustc_mir/build
csmoe Aug 28, 2018
5aae53e
Introduce has_no_projection
csmoe Aug 28, 2018
9ec03b3
Rewrite place workflow in rustc_passes
csmoe Aug 28, 2018
2af0dd0
Rewrite place workflow in rustc_mir/util
csmoe Aug 28, 2018
6045a8a
move `PlaceContext::Copy` check later, once type is fully computed
nikomatsakis Aug 30, 2018
e912a26
Trip unneeded has_no_projection checking
csmoe Aug 31, 2018
13a9c20
Separate inner closure to fn
csmoe Aug 31, 2018
d5095f6
Fix mutability capture from place
csmoe Sep 1, 2018
6f8aa3d
mutability_errors: rustfmt
csmoe Sep 6, 2018
7da7205
returns immediately on references error
csmoe Sep 6, 2018
c47b2a9
guarantee working no projection place when reporting mutability error
csmoe Sep 6, 2018
8616672
working on on projection place when visit_assign
csmoe Sep 6, 2018
ba7feb1
guarantee has_no_projection when visit_rvalue
csmoe Sep 6, 2018
552a935
guarantee has_no_projection in uniform_array_moveout
csmoe Sep 6, 2018
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
24 changes: 17 additions & 7 deletions src/librustc/ich/impls_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,28 +290,38 @@ impl<'a, 'gcx, T> HashStable<StableHashingContext<'a>>

impl_stable_hash_for!(enum mir::ValidationOp { Acquire, Release, Suspend(region_scope) });

impl<'a, 'gcx> HashStable<StableHashingContext<'a>> for mir::Place<'gcx> {
impl<'a, 'gcx> HashStable<StableHashingContext<'a>> for mir::PlaceBase<'gcx> {
fn hash_stable<W: StableHasherResult>(&self,
hcx: &mut StableHashingContext<'a>,
hasher: &mut StableHasher<W>) {
mem::discriminant(self).hash_stable(hcx, hasher);
match *self {
mir::Place::Local(ref local) => {
mir::PlaceBase::Local(ref local) => {
local.hash_stable(hcx, hasher);
}
mir::Place::Static(ref statik) => {
mir::PlaceBase::Static(ref statik) => {
statik.hash_stable(hcx, hasher);
}
mir::Place::Promoted(ref promoted) => {
mir::PlaceBase::Promoted(ref promoted) => {
promoted.hash_stable(hcx, hasher);
}
mir::Place::Projection(ref place_projection) => {
place_projection.hash_stable(hcx, hasher);
}
}
}
}

impl<'a, 'tcx> HashStable<StableHashingContext<'a>>
for mir::Place<'tcx>
{
fn hash_stable<W: StableHasherResult>(
&self,
hcx: &mut StableHashingContext<'a>,
hasher: &mut StableHasher<W>,
) {
self.base.hash_stable(hcx, hasher);
self.elems.hash_stable(hcx, hasher);
Copy link
Member

Choose a reason for hiding this comment

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

You might be able to use one of the macros since this is just a struct now.

}
}

impl<'a, 'gcx, B, V, T> HashStable<StableHashingContext<'a>>
for mir::Projection<'gcx, B, V, T>
where B: HashStable<StableHashingContext<'a>>,
Expand Down
213 changes: 146 additions & 67 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ use mir::interpret::{EvalErrorKind, Scalar, Value, ScalarMaybeUndef};
use mir::visit::MirVisitable;
use rustc_apfloat::ieee::{Double, Single};
use rustc_apfloat::Float;
use rustc_data_structures::accumulate_vec::AccumulateVec;
use rustc_data_structures::graph::dominators::{dominators, Dominators};
use rustc_data_structures::graph::{self, GraphPredecessors, GraphSuccessors};
use rustc_data_structures::indexed_vec::{Idx, IndexVec};
Expand All @@ -39,7 +40,7 @@ use syntax::symbol::InternedString;
use syntax_pos::{Span, DUMMY_SP};
use ty::fold::{TypeFoldable, TypeFolder, TypeVisitor};
use ty::subst::{Subst, Substs};
use ty::{self, AdtDef, CanonicalTy, ClosureSubsts, GeneratorSubsts, Region, Ty, TyCtxt};
use ty::{self, AdtDef, CanonicalTy, ClosureSubsts, GeneratorSubsts, Region, Slice, Ty, TyCtxt};
use util::ppaux;

pub use mir::interpret::AssertMessage;
Expand Down Expand Up @@ -1693,8 +1694,16 @@ impl<'tcx> Debug for Statement<'tcx> {

/// A path to a value; something that can be evaluated without
/// changing or disturbing program state.
#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub struct Place<'tcx> {
pub base: PlaceBase<'tcx>,
pub elems: &'tcx Slice<PlaceElem<'tcx>>,
}

impl<'tcx> serialize::UseSpecializedDecodable for &'tcx Slice<PlaceElem<'tcx>> {}

#[derive(Clone, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum Place<'tcx> {
pub enum PlaceBase<'tcx> {
/// local variable
Local(Local),

Expand All @@ -1703,9 +1712,6 @@ pub enum Place<'tcx> {

/// Constant code promoted to an injected static
Promoted(Box<(Promoted, Ty<'tcx>)>),

/// projection out of a place (access a field, deref a pointer, etc)
Projection(Box<PlaceProjection<'tcx>>),
}

/// The def-id of a static, along with its normalized type (which is
Expand All @@ -1731,7 +1737,7 @@ pub struct Projection<'tcx, B, V, T> {
pub elem: ProjectionElem<'tcx, V, T>,
}
Copy link
Member

Choose a reason for hiding this comment

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

The Projection struct should be removed.


#[derive(Clone, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, RustcEncodable, RustcDecodable)]
pub enum ProjectionElem<'tcx, V, T> {
Deref,
Field(Field, T),
Expand Down Expand Up @@ -1779,31 +1785,100 @@ pub type PlaceElem<'tcx> = ProjectionElem<'tcx, Local, Ty<'tcx>>;

newtype_index!(Field { DEBUG_FORMAT = "field[{}]" });

impl<'tcx> Place<'tcx> {
pub fn field(self, f: Field, ty: Ty<'tcx>) -> Place<'tcx> {
self.elem(ProjectionElem::Field(f, ty))
impl<'a, 'tcx> Place<'tcx> {
// projection lives in the last elem.
pub fn projection(&self) -> Option<&PlaceElem> {
self.elems.last()
Copy link
Member

Choose a reason for hiding this comment

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

IMO such a method is a hazard. All users should iterate over the elems vector.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I agree with "hazard", but I think a name like last_projection would be good. Also, can the return type be Option<&'tcx PlaceElem>?

Copy link
Member

@nagisa nagisa Aug 11, 2018

Choose a reason for hiding this comment

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

This looks like a hazard to me as well. Better naming would help here, but I don’t see any reason against just inspecting the slice in-place.

}

pub fn deref(self) -> Place<'tcx> {
self.elem(ProjectionElem::Deref)
pub fn field(
self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
f: Field,
ty: Ty<'tcx>,
) -> Self {
self.elem(tcx, ProjectionElem::Field(f, ty))
}

pub fn downcast(self, adt_def: &'tcx AdtDef, variant_index: usize) -> Place<'tcx> {
self.elem(ProjectionElem::Downcast(adt_def, variant_index))
pub fn deref(self, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be tcx: TyCtxt<'_, '_, 'tcx>

self.elem(tcx, ProjectionElem::Deref)
}

pub fn index(self, index: Local) -> Place<'tcx> {
self.elem(ProjectionElem::Index(index))
pub fn downcast(
self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
adt_def: &'tcx AdtDef,
variant_index: usize,
) -> Self {
self.elem(tcx, ProjectionElem::Downcast(adt_def, variant_index))
}

pub fn elem(self, elem: PlaceElem<'tcx>) -> Place<'tcx> {
Place::Projection(Box::new(PlaceProjection { base: self, elem }))
pub fn index(self, tcx: TyCtxt<'a, 'tcx, 'tcx>, index: Local) -> Self {
self.elem(tcx, ProjectionElem::Index(index))
}

pub fn constant_index(
self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
offset: u32,
min_length: u32,
from_end: bool,
) -> Self {
self.elem(tcx, ProjectionElem::ConstantIndex {
offset, min_length, from_end,
})
}

pub fn subslice(
self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
from: u32,
to: u32,
) -> Self {
self.elem(tcx, ProjectionElem::Subslice {
from, to,
})
}

pub fn elem(
self,
tcx: TyCtxt<'a, 'tcx, 'tcx>,
Copy link
Contributor

Choose a reason for hiding this comment

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

...and here...etc

elem: PlaceElem<'tcx>,
) -> Self {
Place {
base: self.base,
elems: tcx.intern_place_elems(&[elem]),
Copy link
Member

Choose a reason for hiding this comment

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

This replaces the current elems - it should instead have elem at the end.

Copy link
Member

@nagisa nagisa Aug 11, 2018

Choose a reason for hiding this comment

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

Minor nit.

The builder pattern used here suggests that the Place should be built up incrementally, but it seems to me that it would unnecessarily intern every single intermediate projection. That is, the code currently does not expose any way to construct the whole projection in one go, so one would end up doing something along the lines of

let place = Place::local(x);
let place = place.such_projection(...);
let place = place.another_projection(...);
...

and thus having every slice for each intermediate projection interned. Not sure if this is a relevant problem or whether the current code would be able to take advantage of a one-shot method, though.

}
}

pub fn local(local: Local) -> Self {
Place {
base: PlaceBase::Local(local),
elems: Slice::empty(),
}
}

pub fn static_(static_: Static<'tcx>) -> Self {
Place {
base: PlaceBase::Static(box static_),
elems: Slice::empty(),
}
}

pub fn promoted(
promoted: Promoted,
ty: Ty<'tcx>,
) -> Self {
Place {
base: PlaceBase::Promoted(box (promoted, ty)),
elems: Slice::empty(),
}
}
}

impl<'tcx> Debug for Place<'tcx> {
impl<'tcx> Debug for PlaceBase<'tcx> {
fn fmt(&self, fmt: &mut Formatter) -> fmt::Result {
use self::Place::*;
use self::PlaceBase::*;

match *self {
Local(id) => write!(fmt, "{:?}", id),
Expand All @@ -1814,35 +1889,6 @@ impl<'tcx> Debug for Place<'tcx> {
ty
),
Promoted(ref promoted) => write!(fmt, "({:?}: {:?})", promoted.0, promoted.1),
Projection(ref data) => match data.elem {
ProjectionElem::Downcast(ref adt_def, index) => {
write!(fmt, "({:?} as {})", data.base, adt_def.variants[index].name)
}
ProjectionElem::Deref => write!(fmt, "(*{:?})", data.base),
ProjectionElem::Field(field, ty) => {
write!(fmt, "({:?}.{:?}: {:?})", data.base, field.index(), ty)
}
ProjectionElem::Index(ref index) => write!(fmt, "{:?}[{:?}]", data.base, index),
ProjectionElem::ConstantIndex {
offset,
min_length,
from_end: false,
} => write!(fmt, "{:?}[{:?} of {:?}]", data.base, offset, min_length),
ProjectionElem::ConstantIndex {
offset,
min_length,
from_end: true,
} => write!(fmt, "{:?}[-{:?} of {:?}]", data.base, offset, min_length),
ProjectionElem::Subslice { from, to } if to == 0 => {
write!(fmt, "{:?}[{:?}:]", data.base, from)
}
ProjectionElem::Subslice { from, to } if from == 0 => {
write!(fmt, "{:?}[:-{:?}]", data.base, to)
}
ProjectionElem::Subslice { from, to } => {
write!(fmt, "{:?}[{:?}:-{:?}]", data.base, from, to)
}
},
Copy link
Member

Choose a reason for hiding this comment

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

You need to move this in a custom Debug impl for Place.

}
}
}
Expand Down Expand Up @@ -2760,18 +2806,36 @@ impl<'tcx> TypeFoldable<'tcx> for Terminator<'tcx> {

impl<'tcx> TypeFoldable<'tcx> for Place<'tcx> {
fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, folder: &mut F) -> Self {
match self {
&Place::Projection(ref p) => Place::Projection(p.fold_with(folder)),
_ => self.clone(),
Place {
base: self.base.fold_with(folder),
elems: self.elems.fold_with(folder),
csmoe marked this conversation as resolved.
Show resolved Hide resolved
}
}

fn super_visit_with<V: TypeVisitor<'tcx>>(&self, visitor: &mut V) -> bool {
if let &Place::Projection(ref p) = self {
p.visit_with(visitor)
} else {
false
}
self.base.visit_with(visitor) ||
self.elems.visit_with(visitor)
}
}

impl<'tcx> TypeFoldable<'tcx> for PlaceBase<'tcx> {
fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, _folder: &mut F) -> Self {
self.clone()
}

fn super_visit_with<V: TypeVisitor<'tcx>>(&self, _visitor: &mut V) -> bool {
false
}
}

impl<'tcx> TypeFoldable<'tcx> for &'tcx Slice<PlaceElem<'tcx>> {
fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, folder: &mut F) -> Self {
let v = self.iter().map(|p| p.fold_with(folder)).collect::<AccumulateVec<[_; 8]>>();
folder.tcx().intern_place_elems(&v)
}

fn super_visit_with<Vs: TypeVisitor<'tcx>>(&self, visitor: &mut Vs) -> bool {
self.iter().any(|p| p.visit_with(visitor))
}
}

Expand Down Expand Up @@ -2858,37 +2922,52 @@ impl<'tcx> TypeFoldable<'tcx> for Operand<'tcx> {
}
}

impl<'tcx, B, V, T> TypeFoldable<'tcx> for Projection<'tcx, B, V, T>
where
B: TypeFoldable<'tcx>,
V: TypeFoldable<'tcx>,
T: TypeFoldable<'tcx>,
impl<'tcx, V, T> TypeFoldable<'tcx> for ProjectionElem<'tcx, V, T>
where V: TypeFoldable<'tcx>,
T: TypeFoldable<'tcx>
{
fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, folder: &mut F) -> Self {
use mir::ProjectionElem::*;

let base = self.base.fold_with(folder);
let elem = match self.elem {
match *self {
Deref => Deref,
Field(f, ref ty) => Field(f, ty.fold_with(folder)),
Index(ref v) => Index(v.fold_with(folder)),
ref elem => elem.clone(),
};

Projection { base, elem }
}
}

fn super_visit_with<Vs: TypeVisitor<'tcx>>(&self, visitor: &mut Vs) -> bool {
use mir::ProjectionElem::*;

self.base.visit_with(visitor) || match self.elem {
match *self {
Field(_, ref ty) => ty.visit_with(visitor),
Index(ref v) => v.visit_with(visitor),
_ => false,
}
}
}

impl<'tcx, B, V, T> TypeFoldable<'tcx> for Projection<'tcx, B, V, T>
where
B: TypeFoldable<'tcx>,
V: TypeFoldable<'tcx>,
T: TypeFoldable<'tcx>,
{
fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, folder: &mut F) -> Self {

let base = self.base.fold_with(folder);
let elem = self.elem.fold_with(folder);
Projection { base, elem }
}

fn super_visit_with<Vs: TypeVisitor<'tcx>>(&self, visitor: &mut Vs) -> bool {

self.base.visit_with(visitor) ||
self.elem.visit_with(visitor)
}
}

impl<'tcx> TypeFoldable<'tcx> for Field {
fn super_fold_with<'gcx: 'tcx, F: TypeFolder<'gcx, 'tcx>>(&self, _: &mut F) -> Self {
*self
Expand Down
Loading