[nll] change how MIR represents places by csmoe · Pull Request #53247 · rust-lang/rust

csmoe · 2018-08-10T13:12:06Z

nikomatsakis · 2018-08-10T13:17:53Z

cc @eddyb as well

eddyb · 2018-08-10T13:23:33Z

src/librustc/ich/impls_mir.rs

+        hasher: &mut StableHasher<W>,
+    ) {
+        self.base.hash_stable(hcx, hasher);
+        self.elems.hash_stable(hcx, hasher);


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

eddyb · 2018-08-10T13:24:22Z

src/librustc/mir/mod.rs

+impl<'a, 'tcx> Place<'tcx> {
+    // projection lives in the last elem.
+    pub fn projection(&self) -> Option<&PlaceElem> {
+        self.elems.last()


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

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>?

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.

eddyb · 2018-08-10T13:25:00Z

src/librustc/mir/mod.rs

+    ) -> Self {
+        Place {
+            base: self.base,
+            elems: tcx.intern_place_elems(&[elem]),


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

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.

eddyb · 2018-08-10T13:26:47Z

src/librustc/mir/mod.rs

-                ProjectionElem::Subslice { from, to } => {
-                    write!(fmt, "{:?}[{:?}:-{:?}]", data.base, from, to)
-                }
-            },


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

eddyb · 2018-08-10T13:27:49Z

src/librustc/mir/mod.rs

@@ -1731,7 +1737,7 @@ pub struct Projection<'tcx, B, V, T> {
    pub elem: ProjectionElem<'tcx, V, T>,
 }


The Projection struct should be removed.

src/librustc/mir/mod.rs

eddyb · 2018-08-10T13:30:11Z

src/librustc/mir/tcx.rs

+        //
+        // Place: base.[]
+        //             ^^-- no projection
+        //        ^^^^-- PlaceTy


This is inaccurate, it's only applying .c but you also have .a and .b to apply before.

eddyb · 2018-08-10T13:31:20Z

src/librustc/mir/tcx.rs

+    // if no projection returns the place itself,
+    //     Base.[] => Base.[]
+    //          ^^-- no projection
+    pub fn base_place<'cx, 'gcx>(


Probably a bad idea, because of inefficiency - you should be able to rewrite all the algorithms to work on [PlaceElem] instead of the recursive data structure.

eddyb · 2018-08-10T13:32:46Z

@csmoe It seems that there needs to be a bit more work put into avoiding looking at only the last projection element (previously recursive algorithms should now be iterative).

csmoe · 2018-08-10T13:33:40Z

src/librustc_mir/borrow_check/path_utils.rs

+        match place.base {
+            PlaceBase::Promoted(_)
+            | PlaceBase::Static(..) => false,
+            PlaceBase::Local(..) => true,


@eddyb there are plenty of such pattern-matchs on old_place, I'm a bit uncertain this translation, is this correct?

borrow_of_local_data is a recursive function, which means you can make it iterate over the elems.

Furthermore, in this case specifically, if you made it iterative, you'd notice that the elems don't change at all whether the function returns true or false, it just needs to look at the base.

nikomatsakis · 2018-08-10T20:47:47Z

src/librustc/mir/mod.rs


-    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 {


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

nikomatsakis · 2018-08-10T20:48:05Z

src/librustc/mir/mod.rs

-        Place::Projection(Box::new(PlaceProjection { base: self, elem }))
+    pub fn elem(
+        self,
+        tcx: TyCtxt<'a, 'tcx, 'tcx>,


...and here...etc

nikomatsakis · 2018-08-30T13:41:46Z

src/librustc_mir/build/expr/as_rvalue.rs

+                                } else {
+                                    false
+                                },
+                                "Unexpected capture place",


I'm currently getting a failure here, and I think it's because this code is not really equivalent to the old code. I think this is where a split_projection sort of thing would be useful. In any case, the problem here I believe:

If you have something like *x.f -- this has a tree structure like:

deref

field (f)

base: x

but in the code here we will pull off the last projection (*) and then look at arg_place.base, which is x, ignoring the field f that comes in between.

nikomatsakis

So, this seems right, though as I wrote -- I would avoid the massive closure and instead have a proper fn.

nikomatsakis · 2018-08-30T14:44:40Z

src/librustc_mir/borrow_check/places_conflict.rs

-        })
-    })
-}
+    let place_elements_conflict = |tcx: TyCtxt<'_, 'gcx, 'tcx>,


So, stylistically, I would prefer to keep this as a separate function. I really dislike large closures -- it's hard to distinguish what state they access from the surrounding environment.

csmoe · 2018-09-06T08:22:51Z

src/librustc_codegen_llvm/mir/analyze.rs

                        // potentially stopping at non-operand projections,
                        // which would trigger `not_ssa` on locals.
-                        self.visit_place(&proj.base, context, location);
-                        return;


@nikomatsakis how can I rewrite this self-recursive block in iteration? I suspect my continue rewriting is very incorrect.

pnkfelix · 2018-09-24T12:37:16Z

closing as this PR appears to have been superseded by PR #54426

csmoe added 6 commits August 8, 2018 09:29

Introduce new Place in librustc/mir

357bc2c

Intern Place elems

34329c0

Add split_projection/base_place method

bd9f156

Try read Place

85a87a1

Replace with new Place in mir/borrowck

a5c428b

Clean up old Place in rustc_mir

6002649

rust-highfive assigned nikomatsakis Aug 10, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2018

eddyb reviewed Aug 10, 2018

View reviewed changes

src/librustc/mir/mod.rs Show resolved Hide resolved

eddyb reviewed Aug 10, 2018

View reviewed changes

csmoe commented Aug 10, 2018

View reviewed changes

nikomatsakis reviewed Aug 10, 2018

View reviewed changes

csmoe added 3 commits August 17, 2018 18:33

Intern PlaceElem with iterator

b1dbe6f

Remove Projection and impl stablehash for Place with macro

c5a7e57

Rewrite places_conflict

e684ea3

csmoe force-pushed the place branch 4 times, most recently from ed44737 to 2dc5b86 Compare August 22, 2018 03:05

csmoe added 2 commits August 22, 2018 11:15

Rewrite recurseive place traversal to iteration

091802d

Port new Place into llvm codegen

c1fff48

csmoe force-pushed the place branch from 342ba26 to 9255f9f Compare August 28, 2018 12:41

csmoe added 9 commits August 28, 2018 22:20

Rewrite place workflow in rustc_mir/interpret

ec40255

Rewrite place workflow in rustc_mir/borrow_check

7b65566

Rewrite place workflow in rustc_mir/dataflow

a6520ec

Rewrite place workflow in rustc_mir/transform

fea2e88

Rewrite place workflow in rustc_codegen_llvm

244fbb4

Rewrite place workflow in rustc_mir/build

73e86a2

Introduce has_no_projection

5aae53e

Rewrite place workflow in rustc_passes

9ec03b3

Rewrite place workflow in rustc_mir/util

2af0dd0

csmoe force-pushed the place branch from 9255f9f to 2af0dd0 Compare August 28, 2018 14:27

memoryruins added the A-NLL Area: Non-lexical lifetimes (NLL) label Aug 28, 2018

move PlaceContext::Copy check later, once type is fully computed

6045a8a

nikomatsakis reviewed Aug 30, 2018

View reviewed changes

nikomatsakis requested changes Aug 30, 2018

View reviewed changes

Trip unneeded has_no_projection checking

e912a26

csmoe mentioned this pull request Aug 31, 2018

Merge Promoted and Static variants of mir::Place #53848

Closed

csmoe added 8 commits August 31, 2018 20:01

Separate inner closure to fn

13a9c20

Fix mutability capture from place

d5095f6

mutability_errors: rustfmt

6f8aa3d

returns immediately on references error

7da7205

guarantee working no projection place when reporting mutability error

c47b2a9

working on on projection place when visit_assign

8616672

guarantee has_no_projection when visit_rvalue

ba7feb1

guarantee has_no_projection in uniform_array_moveout

552a935

csmoe force-pushed the place branch from 56498fb to 552a935 Compare September 6, 2018 08:02

csmoe commented Sep 6, 2018

View reviewed changes

csmoe mentioned this pull request Sep 21, 2018

[WIP] Take 2: change how MIR represents Place #54426

Closed

pnkfelix closed this Sep 24, 2018

		@@ -1731,7 +1737,7 @@ pub struct Projection<'tcx, B, V, T> {
		pub elem: ProjectionElem<'tcx, V, T>,
		}

Uh oh!

Conversation

csmoe commented Aug 10, 2018

Uh oh!

nikomatsakis commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagisa Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nagisa Aug 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Aug 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pnkfelix commented Sep 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nagisa Aug 11, 2018 •

edited

Loading

nagisa Aug 11, 2018 •

edited

Loading