Stream: t-compiler

Topic: place 2.0


Santiago Pastorino (Feb 11 2019 at 16:34, on Zulip):

@oli @pnkfelix @nikomatsakis starting a thread here to talk about the Place refactor

oli (Feb 11 2019 at 16:36, on Zulip):

all-hands-discussion: https://paper.dropbox.com/doc/Topic-MIR-2.0-and-MIR-Optimizations--AXXCs_fiKzui0tii_ZOXNEJFAg-BwHR7kOhxDwL6vuAUoSTQ

oli (Feb 11 2019 at 16:38, on Zulip):

previous zulip discussion https://rust-lang.zulipchat.com/#narrow/stream/122657-wg-nll/topic/.2352708-new-place

Santiago Pastorino (Feb 11 2019 at 16:54, on Zulip):

so, I've more or less took over the work about NeoPlace and I could continue with that but I've heard there's a new design idea floating around

Santiago Pastorino (Feb 11 2019 at 16:55, on Zulip):

sadly I miss the last All Hands day :(

nagisa (Feb 11 2019 at 17:01, on Zulip):

I do not think we discussed places too much on the last day.

oli (Feb 11 2019 at 17:05, on Zulip):

Well, there are future changes to it that will refactor it further

oli (Feb 11 2019 at 17:06, on Zulip):

we'll still need the Vec<Projection> setup

oli (Feb 11 2019 at 17:08, on Zulip):

So. In order to not waste any of your work, but instead merge it in small steps, we'd first need to add the Iterator interface to Projection which unfolds the recursive datastructure processing into iterative ones

oli (Feb 11 2019 at 17:09, on Zulip):

then you should be able to merge the algorithms that you already ported without having to modify the recursive datastructures

oli (Feb 11 2019 at 17:10, on Zulip):

once we have transformed all recursive algorithms into iterative ones, we should be able to swap out the datastructures without too much churn

Santiago Pastorino (Feb 11 2019 at 17:12, on Zulip):

:+1:

Santiago Pastorino (Feb 11 2019 at 17:13, on Zulip):

@oli should we have a meeting with interested parties or build a document or something to state the work needed clearly?

oli (Feb 11 2019 at 17:13, on Zulip):

A document would be best I think

oli (Feb 11 2019 at 17:14, on Zulip):

Maybe you could start one and list what you already have?

oli (Feb 11 2019 at 17:14, on Zulip):

I'll add a proposed order of things to do and then we can discuss if that is a useful order

Santiago Pastorino (Feb 11 2019 at 19:28, on Zulip):

:+1:

nikomatsakis (Feb 13 2019 at 21:41, on Zulip):

Hmm, @oli, if you are able to take over guiding @Santiago Pastorino (and maybe @csmoe) through these changes, that would be awesome.

Santiago Pastorino (Feb 13 2019 at 22:05, on Zulip):

I owe a document about this

Santiago Pastorino (Feb 13 2019 at 22:05, on Zulip):

gonna do it asap

oli (Feb 14 2019 at 10:58, on Zulip):

@Santiago Pastorino please don't put too much effort into it. Just a listing and some links of your current state is great

oli (Feb 14 2019 at 10:59, on Zulip):

@nikomatsakis will do

Santiago Pastorino (Feb 14 2019 at 11:31, on Zulip):

@oli :+1:

oli (Feb 16 2019 at 20:11, on Zulip):

I am starting a document in https://paper.dropbox.com/doc/Place-2.0--AXuL44Wh7zhtQcgC2JYOuoiNAQ-9NjhX4N9I3dEt6YCJM8Ln

Santiago Pastorino (Feb 16 2019 at 20:14, on Zulip):

@oli great, gonna fill the document on monday

Santiago Pastorino (Feb 16 2019 at 20:14, on Zulip):

well, unless you finish it I guess :)

oli (Feb 16 2019 at 20:15, on Zulip):

no hurry, I just ran out of PRs to write against rustc and actually went back to my TODO list to see what I can do

Santiago Pastorino (Feb 16 2019 at 20:15, on Zulip):

are you documented what's done in the branch or what you want the Place 2.0 to be?

oli (Feb 16 2019 at 20:16, on Zulip):

I'm documenting the Place 2.0 things we talked about at the all hands

oli (Feb 16 2019 at 20:16, on Zulip):

and the plan how to get there

Santiago Pastorino (Feb 16 2019 at 20:16, on Zulip):

great, should I do then what was accomplished in the current PR?

oli (Feb 16 2019 at 20:16, on Zulip):

jup

Santiago Pastorino (Feb 16 2019 at 20:16, on Zulip):

ok

Santiago Pastorino (Feb 18 2019 at 15:27, on Zulip):

@oli starting the document about the current status of things

Santiago Pastorino (Feb 18 2019 at 15:27, on Zulip):

so we can start discussing and start working on it :)

Santiago Pastorino (Feb 18 2019 at 16:27, on Zulip):

@oli https://paper.dropbox.com/doc/Place-2.0-current-PR-status--AXyh0a7OBh0StKNYTmaPcwtxAg-vmbnFv8VkCEuL57QfWWMH

Santiago Pastorino (Feb 18 2019 at 16:27, on Zulip):

have read your document and added a couple of comments there

Santiago Pastorino (Feb 18 2019 at 16:27, on Zulip):

I can continue in the direction stated in my document

Santiago Pastorino (Feb 18 2019 at 16:28, on Zulip):

I'm not stuck in the process I stopped for a bit working on the thing because @nikomatsakis told me that after the last All Hands days there may have been changes to the design

oli (Feb 18 2019 at 16:47, on Zulip):

So, there were only concerns about the viability of doing your PR in one go

oli (Feb 18 2019 at 16:48, on Zulip):

there are further design changes, but they are addons that can come afterwards and that are actually orthogonal imo

Santiago Pastorino (Feb 18 2019 at 16:53, on Zulip):

ok

Santiago Pastorino (Feb 18 2019 at 16:53, on Zulip):

so ... what should I do? :)

Santiago Pastorino (Feb 18 2019 at 16:55, on Zulip):

is there an alternative/better strategy to the one taked in my branch?

oli (Feb 18 2019 at 16:56, on Zulip):

The suggestion was to implement an iterator over Place which gives you the projections in the order that the slice would give us

oli (Feb 18 2019 at 16:56, on Zulip):

then implement the algorithms on top of that

oli (Feb 18 2019 at 16:56, on Zulip):

this way, every algorithm can be ported and merged without having to require a big PR like yours

oli (Feb 18 2019 at 16:57, on Zulip):

Since you already touched a lot of these algorithms, I have a question: Are most of these feasible on an iterator?

oli (Feb 18 2019 at 16:57, on Zulip):

(forward or backward)

Santiago Pastorino (Feb 18 2019 at 16:58, on Zulip):

most of the algorithms were not implemented by me

Santiago Pastorino (Feb 18 2019 at 16:58, on Zulip):

so, basically the work was, build the structure and implement the algorithms that was done by @csmoe

Santiago Pastorino (Feb 18 2019 at 16:59, on Zulip):

then I took over when we started to make use of NeoPlace and start to replace the new structure with the current old Place

Santiago Pastorino (Feb 18 2019 at 17:00, on Zulip):

@oli maybe I should just start from scratch and do what you're mentioning and see what happens

Santiago Pastorino (Feb 18 2019 at 17:00, on Zulip):

the algorithms are there so, most of the stuff would be copying and pasting

Santiago Pastorino (Feb 18 2019 at 17:01, on Zulip):

can use the opportunity to better review the algorithms, when I was working on the thing I found some issues on a couple

oli (Feb 18 2019 at 17:01, on Zulip):

Presumably all the neoplace algorithms can be copy pasted from your branch, yes

Santiago Pastorino (Feb 18 2019 at 17:03, on Zulip):

ok, so I guess I should try that then :)

oli (Feb 18 2019 at 17:03, on Zulip):

Let me look at the branches again and summon eddyb XD I'll do that tomorrow

Santiago Pastorino (Feb 18 2019 at 17:04, on Zulip):

:+1:

oli (Feb 18 2019 at 17:04, on Zulip):

I don't want you to waste any time, so let's have a closer look first

Santiago Pastorino (Feb 18 2019 at 17:04, on Zulip):

makes sense

Santiago Pastorino (Feb 18 2019 at 17:06, on Zulip):

@oli actually, unsure if I'm understanding correctly the iterator idea

Santiago Pastorino (Feb 18 2019 at 17:07, on Zulip):

why would that make things be easier and mergeable in tinier steps?

Santiago Pastorino (Feb 18 2019 at 17:07, on Zulip):

I may have understood the idea wrong

oli (Feb 18 2019 at 19:14, on Zulip):

because we wouldn't have to change the datastructure

oli (Feb 18 2019 at 19:15, on Zulip):

Note that I'm just repeating what was said, I have not looked at this in enough detail to have an educated opinion

oli (Feb 20 2019 at 10:07, on Zulip):

So I've looked into your PR. I think we can start pulling out some small changes and merge them without having big diffs and making your life easier by simplifying rebases

oli (Feb 20 2019 at 10:08, on Zulip):

I've added it to your paper

oli (Feb 20 2019 at 10:08, on Zulip):

What do you think?

Santiago Pastorino (Feb 20 2019 at 10:27, on Zulip):

@oli sounds good

Santiago Pastorino (Feb 20 2019 at 10:28, on Zulip):

I still don't understand how the iterator idea makes things easier

Santiago Pastorino (Feb 20 2019 at 10:28, on Zulip):

and what things are made easier by that

Santiago Pastorino (Feb 20 2019 at 10:28, on Zulip):

because I still need to replace Place with NeoPlace at some point in the MIR

Santiago Pastorino (Feb 20 2019 at 10:28, on Zulip):

and all that

Santiago Pastorino (Feb 20 2019 at 10:29, on Zulip):

or by easier you meant that are less conflicts?

Santiago Pastorino (Feb 20 2019 at 10:29, on Zulip):

to be honest conflicts weren't bad

oli (Feb 20 2019 at 10:29, on Zulip):

The thing is, each of these steps are single PRs that can be merged

oli (Feb 20 2019 at 10:29, on Zulip):

well, but reviewing the entire PR is bad ;)

oli (Feb 20 2019 at 10:29, on Zulip):

(I'm aware I created the miri PR last year that was way worse)

Santiago Pastorino (Feb 20 2019 at 10:30, on Zulip):

yeah

oli (Feb 20 2019 at 10:30, on Zulip):

reviewing smaller PRs is much easier, and we can't do the Place -> NeoPlace in master, it's too expensive I think

oli (Feb 20 2019 at 10:30, on Zulip):

though it might be worth measuring that

Santiago Pastorino (Feb 20 2019 at 10:30, on Zulip):

yeah it would need to be merged when there's no NeoPlace in the PR

Santiago Pastorino (Feb 20 2019 at 10:31, on Zulip):

so at the very last minute

oli (Feb 20 2019 at 10:33, on Zulip):

We could do a test-PR for perf where all that happens is that you have the conversions, and drop the result immediately (after sending it into a black_box)

oli (Feb 20 2019 at 10:33, on Zulip):

if that's not too expensive, then we can just do the PR that you have in small steps

oli (Feb 20 2019 at 10:34, on Zulip):

but I do fear that it'll show up significantly

Santiago Pastorino (Feb 20 2019 at 10:37, on Zulip):

yeah, the idea of this PR was not to have it merge by intermediate steps

Santiago Pastorino (Feb 20 2019 at 10:38, on Zulip):

it will be bad I think

Santiago Pastorino (Feb 20 2019 at 10:38, on Zulip):

anyway, let's switch to your idea :)

Santiago Pastorino (Feb 20 2019 at 10:50, on Zulip):

@oli will ping you in 3/4 hours when I'm back so we can start with this

Santiago Pastorino (Feb 20 2019 at 12:39, on Zulip):

@oli kind of back

oli (Feb 20 2019 at 12:39, on Zulip):

:wave:

Santiago Pastorino (Feb 20 2019 at 12:39, on Zulip):

why did you remove this from the paper doc ...

Santiago Pastorino (Feb 20 2019 at 12:39, on Zulip):
// this might do the trick for iterating in the same direction that we'd recurse
// though this is often not what we want, we want the reverse (iterating in source direction)
impl<'a, 'tcx> Iterator for &'a PlaceProjection<'tcx> {
    type Item = &'a PlaceElem<'tcx>;
    fn next(&mut self) -> Option<Self::Item> {
        match &self.base {
            Place::Projection(p) => {
                *self = p;
                Some(&p.elem)
            },
            _ => None,
        }
    }
}
oli (Feb 20 2019 at 12:39, on Zulip):

oh, because it seemed useless

oli (Feb 20 2019 at 12:39, on Zulip):

it's the wrong direction

Santiago Pastorino (Feb 20 2019 at 12:39, on Zulip):

ok

oli (Feb 20 2019 at 12:39, on Zulip):

but you can keep it if there are any algorithms that need it

Santiago Pastorino (Feb 20 2019 at 12:40, on Zulip):

so ... let me read a bit and try to understand the directions I need to take

Santiago Pastorino (Feb 20 2019 at 12:40, on Zulip):

the idea would be to ...

Santiago Pastorino (Feb 20 2019 at 12:40, on Zulip):

1. add a PlaceBase and use that from Place

Santiago Pastorino (Feb 20 2019 at 12:41, on Zulip):

2. add an iterate method to Place

Santiago Pastorino (Feb 20 2019 at 12:41, on Zulip):

3. use iterate to implement the algorithms

Santiago Pastorino (Feb 20 2019 at 12:42, on Zulip):

4. add NeoPlace with iterate method

Santiago Pastorino (Feb 20 2019 at 12:42, on Zulip):

right?

oli (Feb 20 2019 at 12:42, on Zulip):

I'm not sure about 4.

oli (Feb 20 2019 at 12:42, on Zulip):

it might be that we'll have to go from Place to NeoPlace in one go

Santiago Pastorino (Feb 20 2019 at 12:42, on Zulip):

ok, we can see as we go

Santiago Pastorino (Feb 20 2019 at 12:42, on Zulip):

1 is clear

Santiago Pastorino (Feb 20 2019 at 12:43, on Zulip):

about 2, couldn't that hurt performance?

oli (Feb 20 2019 at 12:43, on Zulip):

2 won't hurt perf, because the method would be unused ;)

oli (Feb 20 2019 at 12:43, on Zulip):

3. could hurt perf

Santiago Pastorino (Feb 20 2019 at 12:43, on Zulip):

ahh well yeah :)

oli (Feb 20 2019 at 12:44, on Zulip):

but you can start in the code that uses unroll_place

oli (Feb 20 2019 at 12:44, on Zulip):

that should be completely free from perf regressions

oli (Feb 20 2019 at 12:44, on Zulip):

since it already does this

Santiago Pastorino (Feb 20 2019 at 12:44, on Zulip):

yeah

Santiago Pastorino (Feb 20 2019 at 12:44, on Zulip):

and also, the idea is that that iterator iterates over PlaceProjections?

Santiago Pastorino (Feb 20 2019 at 12:44, on Zulip):

what about Base?

Santiago Pastorino (Feb 20 2019 at 12:45, on Zulip):

to be honest right now I don't remember how is the code layed out in that sense

Santiago Pastorino (Feb 20 2019 at 12:45, on Zulip):

saw some bits of code were you were getting a collection with the projections and the base, with NeoPlace I think that was not true

Santiago Pastorino (Feb 20 2019 at 12:45, on Zulip):

but just trying to understand what you mean exactly by that iterator

oli (Feb 20 2019 at 12:46, on Zulip):

it iterates over PlaceProjections but you have access to the place the entire time

oli (Feb 20 2019 at 12:46, on Zulip):

The iterator is basically just a wrapper around the single linked list produces by unroll_place

oli (Feb 20 2019 at 12:46, on Zulip):

just giving us a convenient API

Santiago Pastorino (Feb 20 2019 at 12:46, on Zulip):

ahh ok ok

oli (Feb 20 2019 at 12:46, on Zulip):

you can start without the iterator if you want

Santiago Pastorino (Feb 20 2019 at 12:46, on Zulip):

I see

oli (Feb 20 2019 at 12:47, on Zulip):

just putting the unroll_placeonto Place

Santiago Pastorino (Feb 20 2019 at 12:47, on Zulip):

ok, let me go over all this 3 points and we can discuss after it

Santiago Pastorino (Feb 20 2019 at 12:47, on Zulip):

I won't be able until a bit later today

Santiago Pastorino (Feb 20 2019 at 12:47, on Zulip):

but anyway will ping you

Santiago Pastorino (Feb 21 2019 at 16:04, on Zulip):

@oli btw, just started to add PlaceBase

Santiago Pastorino (Feb 21 2019 at 16:04, on Zulip):

going through the compile errors

Santiago Pastorino (Feb 21 2019 at 16:04, on Zulip):

:)

oli (Feb 21 2019 at 16:05, on Zulip):

Thanks so much for taking point on this

Santiago Pastorino (Feb 21 2019 at 16:06, on Zulip):

310 errors :')

csmoe (Feb 21 2019 at 16:07, on Zulip):

@Santiago Pastorino may I have your branch url? or you just started locally.

Santiago Pastorino (Feb 21 2019 at 16:07, on Zulip):

locally

csmoe (Feb 21 2019 at 16:08, on Zulip):

okay

Santiago Pastorino (Feb 21 2019 at 16:08, on Zulip):

but I'm going to push as soon as there's something

oli (Feb 21 2019 at 16:22, on Zulip):

I just had an idea for another intermediate step (added to https://paper.dropbox.com/doc/Place-2.0-current-PR-status--AX~vQTEYLbebQ8o~qNmVWNH9Ag-vmbnFv8VkCEuL57QfWWMH#:uid=518781409188999927847299&h2=oli-obk-musings-for-incrementa)

oli (Feb 21 2019 at 16:23, on Zulip):

We can rip out the base structure out of the projection structure and just have a "leaf" enum variant where the actual base would have been

Santiago Pastorino (Feb 21 2019 at 16:33, on Zulip):

you meant, this part ...

Santiago Pastorino (Feb 21 2019 at 16:33, on Zulip):
struct Place {
    base: PlaceBase,
    projection: PlaceProjection,
}
enum PlaceProjection {
    Base,
    Projection(Box<PlaceProjection>),
    Deref,
    Index(..),
    ...
}
oli (Feb 21 2019 at 16:39, on Zulip):

jup

Santiago Pastorino (Feb 21 2019 at 16:40, on Zulip):

so, one drawback of that is ... changing how you structure Place gives your ~ 300 errors :)

Santiago Pastorino (Feb 21 2019 at 16:40, on Zulip):

I can go over all of them

Santiago Pastorino (Feb 21 2019 at 16:41, on Zulip):

I was just thinking what are the benefits and drawbacks of splitting step 1 and step 3

Santiago Pastorino (Feb 21 2019 at 16:41, on Zulip):

or doing them together so I go through the 300 errors at once

Santiago Pastorino (Feb 21 2019 at 16:42, on Zulip):

in the first step I need to do changes like ...

Santiago Pastorino (Feb 21 2019 at 16:42, on Zulip):
-            mir::Place::Promoted(ref promoted) => {
+            mir::Place::Base(mir::PlaceBase::Promoted(ref promoted)) => {
Santiago Pastorino (Feb 21 2019 at 16:42, on Zulip):

but given that now is a struct I'd need to change again

Santiago Pastorino (Feb 21 2019 at 16:42, on Zulip):

to be clear, I'm fine to doing all that twice

Santiago Pastorino (Feb 21 2019 at 16:42, on Zulip):

no worries

Santiago Pastorino (Feb 21 2019 at 16:42, on Zulip):

will practice a LOT my vim skills :P

Santiago Pastorino (Feb 21 2019 at 16:43, on Zulip):

but just rising the issue :)

oli (Feb 21 2019 at 19:27, on Zulip):

I'm fine with doing those in one sweep

oli (Feb 21 2019 at 19:27, on Zulip):

you're right, this is gonna touch all of the same code

Santiago Pastorino (Feb 21 2019 at 19:37, on Zulip):

I'm kind of 60% with the task 1, so I guess I can just finish that and use the practice to do it again :)

Santiago Pastorino (Feb 22 2019 at 04:29, on Zulip):

@oli https://github.com/rust-lang/rust/pull/58631

RalfJ (Feb 22 2019 at 09:39, on Zulip):

At the all-hands, it was also suggested that Deref should move out of PlaceProjection and into PlaceBase. is that still planned (as a next step, I suppose)?

oli (Feb 22 2019 at 09:43, on Zulip):

that's in the second document

oli (Feb 22 2019 at 09:44, on Zulip):

and it is ignored for this refactoring, because moving Deref out is a change that has much deeper consequences than just a shallow restructuring of a datastructure

Santiago Pastorino (Feb 22 2019 at 11:32, on Zulip):

@oli I'd love to have a way of running rustfmt over the git diff output :)

Santiago Pastorino (Feb 22 2019 at 11:32, on Zulip):

saw your review, gonna fix it as soon as I can

Santiago Pastorino (Feb 22 2019 at 11:40, on Zulip):

@oli also https://github.com/rust-lang/rust/pull/58631#discussion_r259308597

Santiago Pastorino (Feb 23 2019 at 04:17, on Zulip):

@oli force pushed https://github.com/rust-lang/rust/pull/58631

oli (Feb 23 2019 at 08:23, on Zulip):

Awesome. Feel free to rebase and r=me if other PRs cause merge conflicts

centril (Feb 23 2019 at 08:45, on Zulip):

I p=1-ed in case you get conflicts so you dont have to do it so many times

Santiago Pastorino (Feb 24 2019 at 17:26, on Zulip):

@oli https://github.com/rust-lang/rust/pull/58631 saw some failures on src/tools/clipy and src/tools/miri, gonna fix when I have some time with the computer, probably tomorrow

Santiago Pastorino (Feb 24 2019 at 21:36, on Zulip):

@oli just got some minutes to fix this, how are things on src/tools adapter because those live in a different repo, I could provide a PR to each of those repos and as part of this PR bump the deps?

Santiago Pastorino (Feb 24 2019 at 21:36, on Zulip):

is that what I should do?

Matthew Jasper (Feb 24 2019 at 22:06, on Zulip):

Wait until the beta cut, then you will be able to temporarily break them.

Santiago Pastorino (Feb 24 2019 at 22:22, on Zulip):

@Matthew Jasper when is the next beta cut? is there a release calendar out there?

Santiago Pastorino (Feb 24 2019 at 22:23, on Zulip):

I see the Rust release calendar that says the release is on feb 28th

Santiago Pastorino (Feb 24 2019 at 22:24, on Zulip):

but that’s for stable when exactly beta happens?

Matthew Jasper (Feb 24 2019 at 22:58, on Zulip):

Around the same time, release cut is usually on the Monday, but it might be a bit late this time due to some bugs. Beta cut is then later that week usually.

Santiago Pastorino (Feb 24 2019 at 23:53, on Zulip):

:+1:

Santiago Pastorino (Feb 25 2019 at 20:03, on Zulip):

@oli do you agree with what we've said with Matthew?

oli (Feb 26 2019 at 07:52, on Zulip):

Yea, just waiting until the beta is cut sgtm, we'll prioritze the PR to reduce rebasing pains

Santiago Pastorino (Feb 26 2019 at 13:14, on Zulip):

@oli but should I already send PRs to tools repos?

oli (Feb 26 2019 at 13:24, on Zulip):

Oh, that's a great idea, that will speed up fixing the tools after the breakage from your PR.

Santiago Pastorino (Feb 26 2019 at 13:56, on Zulip):

ok, will do that in a bit

Santiago Pastorino (Feb 26 2019 at 18:05, on Zulip):

@oli https://github.com/rust-lang/miri/pull/647 and https://github.com/rust-lang/rust-clippy/pull/3823

Santiago Pastorino (Feb 26 2019 at 18:11, on Zulip):

and btw, rebased https://github.com/rust-lang/rust/pull/58631 because it was not mergeable anymore

Santiago Pastorino (Feb 26 2019 at 20:01, on Zulip):

@oli meanwhile I wait for all this I was going to continue with what is next

Santiago Pastorino (Feb 26 2019 at 20:02, on Zulip):

basically is this iterate idea you had using the unroll_place fn

Santiago Pastorino (Feb 26 2019 at 20:02, on Zulip):

can you explain a bit more what your strategy is about and what do you want to accomplish with it?

Santiago Pastorino (Feb 26 2019 at 20:04, on Zulip):

what's the connection between this for instance with PlaceComponentsIter?

Santiago Pastorino (Feb 26 2019 at 20:04, on Zulip):

I guess if you can start explaining from the beginning would be great :)

oli (Feb 26 2019 at 20:05, on Zulip):

hmm... I did not know about PlaceComponentsIter

oli (Feb 26 2019 at 20:06, on Zulip):

looks to me like this is pretty much what we want

oli (Feb 26 2019 at 20:07, on Zulip):

I'm confused

oli (Feb 26 2019 at 20:07, on Zulip):

what was the code that I was looking at before...

oli (Feb 26 2019 at 20:08, on Zulip):

Ok, so basically that step is already done. I guess you can pull the unroll_place function onto Place and make it an unroll method

oli (Feb 26 2019 at 20:08, on Zulip):

Then have a look at which algorithms can be implemented via unroll instead of recursion

Santiago Pastorino (Feb 26 2019 at 20:23, on Zulip):

ok, so we want to move all that into Place

Santiago Pastorino (Feb 26 2019 at 20:23, on Zulip):

because there's a lot of things there that are only related to that

Santiago Pastorino (Feb 26 2019 at 20:24, on Zulip):

in particular here https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check/places_conflict.rs is where unroll_place is defined and only used

Santiago Pastorino (Feb 26 2019 at 20:25, on Zulip):

so, we just move the function to Place as unroll and call that from there?

Santiago Pastorino (Feb 26 2019 at 20:30, on Zulip):

in particular that iterator exist for the only purpose of being used there

Santiago Pastorino (Feb 26 2019 at 20:31, on Zulip):

so I guess I can move that altogether to where Place is defined and use unroll there

Santiago Pastorino (Feb 26 2019 at 20:31, on Zulip):

@oli let me know if this is more or less your idea

oli (Feb 26 2019 at 20:32, on Zulip):

jup that fits my idea

Santiago Pastorino (Feb 26 2019 at 22:25, on Zulip):

@oli another thing is if I should push all this to the same PR or we should just wait until beta to get the first PR and then come back to this?

Santiago Pastorino (Feb 26 2019 at 22:26, on Zulip):

my guess is that I may be able to even change Place to be a struct in that 1 week window

Santiago Pastorino (Feb 26 2019 at 22:26, on Zulip):

so everything is a bit quicker

Santiago Pastorino (Feb 26 2019 at 22:26, on Zulip):

maybe these 2 changes some algorithms that use unroll instead of recursion if any and then change Place again to be a struct as you proposed

oli (Feb 27 2019 at 06:05, on Zulip):

I wouldn't include unroll changes in the PR, but struct changes are fine with me

oli (Feb 27 2019 at 06:05, on Zulip):

We can start merging again

oli (Feb 27 2019 at 06:05, on Zulip):

Beta has been cut

oli (Feb 27 2019 at 06:07, on Zulip):

Beta has been cut

oli (Feb 27 2019 at 06:08, on Zulip):

Beta has been cut

Santiago Pastorino (Feb 27 2019 at 13:22, on Zulip):

@oli then merge what we have for now :)

Santiago Pastorino (Feb 27 2019 at 13:23, on Zulip):

@oli this https://github.com/rust-lang/rust/pull/58631 is ready to r+

Santiago Pastorino (Feb 27 2019 at 13:24, on Zulip):

the unroll changes are ready too but I didn't push them there

Santiago Pastorino (Feb 27 2019 at 15:31, on Zulip):

and also ...

Santiago Pastorino (Feb 27 2019 at 15:31, on Zulip):
  - Proposed structure is like this (inspired by something @eddyb said, and I imagine that they will have suggestions too):


    struct Place<'tcx> {
        base: PlaceBase<'tcx>,
        elem: &'tcx Slice<ProjectionElem<'tcx>>,
    }

    enum PlaceBase<'tcx> {
        /// local variable
        Local(Local),

        /// static or static mut variable
        Static(Box<Static<'tcx>>),

        /// Constant code promoted to an injected static
        Promoted(Box<(Promoted, Ty<'tcx>)>),
    }
Santiago Pastorino (Feb 27 2019 at 15:31, on Zulip):

@oli @eddyb are we sold on this?

Santiago Pastorino (Feb 27 2019 at 15:31, on Zulip):

starting to change code to that

Santiago Pastorino (Feb 27 2019 at 15:32, on Zulip):

seems good to me, the thing is ... isn't that more or less how NeoPlace was going to look like?

Santiago Pastorino (Feb 27 2019 at 15:33, on Zulip):

won't that mean that I'm going to change all the algorithms at once to make all this compile?

Santiago Pastorino (Feb 27 2019 at 15:40, on Zulip):

I wouldn't include unroll changes in the PR, but struct changes are fine with me

@oli also, I guess this is wrong

Santiago Pastorino (Feb 27 2019 at 15:40, on Zulip):

if I do the struct changes before the unroll thing and mainly make all the algorithms that are recursive use unroll, my guess is that when I go and change to struct I'm going to need to change a lot of things

Santiago Pastorino (Feb 27 2019 at 15:41, on Zulip):

it seems that we really want to make unroll and change the recursive algorithms to use unroll before we do the struct

Santiago Pastorino (Feb 27 2019 at 15:41, on Zulip):

but anyway, the struct part, given that is not recursive anymore, it's probably going to be huge

Santiago Pastorino (Feb 27 2019 at 15:47, on Zulip):

to be clear, at the beginning I didn't notice this elem: &'tcx Slice<ProjectionElem<'tcx>>, there's ProjectionElem there and not PlaceProjection

Santiago Pastorino (Feb 27 2019 at 15:48, on Zulip):

which is perfect but makes the change need to change all the algorithms at once

Santiago Pastorino (Feb 27 2019 at 15:50, on Zulip):

anyway, the iterate idea seems fine at glance, it just that it has confused me a bit the last thing @oli have said about changing to struct before doing the unroll (iterate) thing

Santiago Pastorino (Feb 27 2019 at 15:50, on Zulip):

that seems harder and seems like, let's make all the change at once

nikomatsakis (Feb 27 2019 at 15:56, on Zulip):

PS, I'm skimming this paper document, is this still the best place to catch up?

Santiago Pastorino (Feb 27 2019 at 16:03, on Zulip):

yep + this paper doc + this chat :)

nikomatsakis (Feb 27 2019 at 16:04, on Zulip):

The steps I see are:

One thing that I brought up with with @Santiago Pastorino a few times is that MIR construction will want to be changed to be efficient. I think it might make sense, after adding the iterate method, to try and introduce a PlaceBuilder and port MIR construction to use it. This would be relatively easy to write atop the existing Place.

Once that is done, I think that we could probably do a "final step" that changes Place to the new structure -- the producers won't have to be changed, as they are now using PlaceBuilder. Most consumers won't have to be changed, as they are now using iterate. Only the tree-based consumers will have to be updated, and (if we use the slice representation I was suggesting) they can be ported with relative ease I think. (I'm not sure how many there are.)

csmoe (Feb 27 2019 at 16:05, on Zulip):

I successfully rewrote the place_confilct without unroll in the old PR with the new Place. it was kind easy, but not sure how it will be with place2.0

nikomatsakis (Feb 27 2019 at 16:05, on Zulip):

OK, well, see my two cents there @Santiago Pastorino :point_up:

tl;dr I think the first 2 steps seem good, but I suspect we can insert some transition steps before making the final conversion and things will go more smoothly.

Santiago Pastorino (Feb 27 2019 at 16:08, on Zulip):

yes

Santiago Pastorino (Feb 27 2019 at 16:08, on Zulip):

seems good

oli (Feb 27 2019 at 17:30, on Zulip):

The idea of the base,projection_tree pair is that it doesn't change how the recursive algorithms work, but enormously simplifies all the algorithms that just care about the base

oli (Feb 27 2019 at 17:31, on Zulip):

It is also closer to the final (base, projection_slice) structure

oli (Feb 27 2019 at 17:36, on Zulip):

I haven't looked at construction at all so far, I will do that and sketch some ideas

Santiago Pastorino (Feb 27 2019 at 17:39, on Zulip):

@oli the struct in the doc says Slice, you meant to have the recursive form there?

Santiago Pastorino (Feb 27 2019 at 17:40, on Zulip):
    struct Place<'tcx> {
        base: PlaceBase<'tcx>,
        elem: &'tcx Slice<ProjectionElem<'tcx>>,
    }

    enum PlaceBase<'tcx> {
        /// local variable
        Local(Local),

        /// static or static mut variable
        Static(Box<Static<'tcx>>),

        /// Constant code promoted to an injected static
        Promoted(Box<(Promoted, Ty<'tcx>)>),
    }
Santiago Pastorino (Feb 27 2019 at 17:40, on Zulip):

ProjectionElem is the way it should look like when is not recursive anymore

Santiago Pastorino (Feb 27 2019 at 17:41, on Zulip):

PlaceProjection is the current recursive form

oli (Feb 27 2019 at 17:52, on Zulip):

That's the final form that we want to have. As a first step, just having the old ProjectionElem(without even the slice) there is what i meant. I'll elaborate in the doc when i get back on a PC

Santiago Pastorino (Feb 27 2019 at 17:52, on Zulip):

ahh yeah

Santiago Pastorino (Feb 27 2019 at 17:53, on Zulip):

because I was confused that the third step says ... add that

Santiago Pastorino (Feb 27 2019 at 18:18, on Zulip):

@oli btw, I was mentioning this to @nikomatsakis too, should we have a synced Zulip meeting where we can lay out a plan for this that we all agree?

Santiago Pastorino (Feb 27 2019 at 18:19, on Zulip):

my main goal is to better understand what we are going to do, for now I'm following blindly because I don't understand what each step is for

Santiago Pastorino (Feb 27 2019 at 18:19, on Zulip):

I mean, is not exactly that I don't understand

Santiago Pastorino (Feb 27 2019 at 18:20, on Zulip):

maybe it's just because there are some things that needs clarification

Santiago Pastorino (Feb 27 2019 at 18:20, on Zulip):

but I don't see/understand the 100% of the idea yet

oli (Feb 27 2019 at 19:08, on Zulip):

Oh, sorry. Please stop me from now on if I go off on an explanation that isn't helpful for comprehending what i mean

oli (Feb 27 2019 at 19:08, on Zulip):

Just ask any question you have or ask for a reexplanation. I'll happily provide that

oli (Feb 27 2019 at 19:09, on Zulip):

If a sync meeting helps, we can setup a time

Santiago Pastorino (Feb 27 2019 at 19:13, on Zulip):

@oli thanks!, after talking a bit with you and @nikomatsakis I guess I have some stuff to play with for a bit

nikomatsakis (Feb 27 2019 at 20:54, on Zulip):

The idea of the base,projection_tree pair is that it doesn't change how the recursive algorithms work, but enormously simplifies all the algorithms that just care about the base

@oli maybe I didn't read closely enough, is one of the "projection tree" parts just a Base unit variant or something? I'm not convinced that this step is easier than adopting a slice that can be made into a tree by subslicing, if we lay the groundwork first. That said, it seems like the first step (moving to an iterator) is clear, and I personally think introducing a "builder" or something would be a good second step, so it's some time before this becomes a prime concern.

oli (Feb 27 2019 at 22:16, on Zulip):

@nikomatsakis the reason i wanted that intermediate step is the base field, which we'll have in the end, too. The step to a slice would then be limited to the projection field. I think it's an orthogonal refactoring to the slicing/builder scheme, so we can do it afterwards (if even necessary anymore)

Santiago Pastorino (Feb 28 2019 at 19:25, on Zulip):

@oli @centril https://github.com/rust-lang/rust/pull/58631 conflicts again in the PR :(

Santiago Pastorino (Feb 28 2019 at 19:25, on Zulip):

going to rebase again

dlrobertson (Feb 28 2019 at 19:26, on Zulip):

sorry, my bad :(

dlrobertson (Feb 28 2019 at 19:28, on Zulip):

That's due to the refactoring of each of the match arms in codegen_terminator into a separate method

Santiago Pastorino (Feb 28 2019 at 19:30, on Zulip):

no worries, it's fine

Santiago Pastorino (Feb 28 2019 at 19:31, on Zulip):

just wondering how to escape that conflicts :)

Santiago Pastorino (Feb 28 2019 at 23:12, on Zulip):

@oli @centril I've just force pushed https://github.com/rust-lang/rust/pull/58631

centril (Feb 28 2019 at 23:14, on Zulip):

I haven't actually looked at the PR so ill let Oliver r+ and stuff

Santiago Pastorino (Mar 01 2019 at 01:46, on Zulip):

the main question is, how to make this merge quickly

Santiago Pastorino (Mar 01 2019 at 01:47, on Zulip):

queue seems to be taking a couple of days and when it's about to be merged it ends with conflicts

centril (Mar 01 2019 at 02:31, on Zulip):

@Santiago Pastorino you p=20 the PR :slight_smile:

Santiago Pastorino (Mar 01 2019 at 04:03, on Zulip):

I'm not sure I have the rights

centril (Mar 01 2019 at 04:05, on Zulip):

@Santiago Pastorino I did it

oli (Mar 01 2019 at 14:01, on Zulip):

whoo, we are live!

Santiago Pastorino (Mar 01 2019 at 14:33, on Zulip):

@oli a couple of things

Santiago Pastorino (Mar 01 2019 at 14:34, on Zulip):

first there was a conflict on miri PR, I've force pushed some minutes ago

Santiago Pastorino (Mar 01 2019 at 14:34, on Zulip):

the other important thing is ... should I quickly go towards the move into a struct before this break everything window closes?

Santiago Pastorino (Mar 01 2019 at 14:35, on Zulip):

as you may have seen I've done some stuff related to iterators but I didn't review those things that much

Santiago Pastorino (Mar 01 2019 at 14:35, on Zulip):

anyway, I guess I could do first the struct move thing and have that merge before this window closes?

oli (Mar 01 2019 at 15:26, on Zulip):

This "break everything window" closes only for a week every 6 weeks

oli (Mar 01 2019 at 15:26, on Zulip):

so I wouldn't worry

Santiago Pastorino (Mar 01 2019 at 15:28, on Zulip):

@oli but I think I can do the struct thing quickly

Santiago Pastorino (Mar 01 2019 at 15:28, on Zulip):

so the real questions is ... should I do the struct thing first or the iterate thing?

Santiago Pastorino (Mar 01 2019 at 15:29, on Zulip):

@oli and btw, my prs to miri and rust-clippy both are failing because it's using, I guess a previous rustc

Santiago Pastorino (Mar 01 2019 at 15:29, on Zulip):

how do one change that?

oli (Mar 01 2019 at 15:29, on Zulip):

miri: wait a day

oli (Mar 01 2019 at 15:29, on Zulip):

clippy should work immediately

Santiago Pastorino (Mar 01 2019 at 15:29, on Zulip):

ok, do I need to do something else with that then?

Santiago Pastorino (Mar 01 2019 at 15:30, on Zulip):

or just wait and you are going to merge?

oli (Mar 01 2019 at 15:30, on Zulip):

I'm just waiting for travis, then I'll r+ it

oli (Mar 01 2019 at 15:30, on Zulip):

yes, you don't need to do anything further. Thanks for fixing it!

Santiago Pastorino (Mar 01 2019 at 15:30, on Zulip):

ok that with clippy but what with miri

Santiago Pastorino (Mar 01 2019 at 15:30, on Zulip):

you need to kick the ci I guess?

oli (Mar 01 2019 at 15:30, on Zulip):

with miri just wait until tomorrow and then I'll restart CI and merge it

Santiago Pastorino (Mar 01 2019 at 15:31, on Zulip):

cool

Santiago Pastorino (Mar 01 2019 at 15:31, on Zulip):

so the real questions is ... should I do the struct thing first or the iterate thing?

in this regard I can go whatever you prefer

Santiago Pastorino (Mar 01 2019 at 15:31, on Zulip):

it's more or less the same for me

Santiago Pastorino (Mar 01 2019 at 15:31, on Zulip):

I'm maybe tempted to do merge the struct thing first

Santiago Pastorino (Mar 01 2019 at 15:31, on Zulip):

given the pain that it is

Santiago Pastorino (Mar 01 2019 at 15:31, on Zulip):

and so we don't need to wait for 6 weeks

oli (Mar 01 2019 at 15:32, on Zulip):

sgtm. You are doing the impls, so you are calling the shots!

Santiago Pastorino (Mar 01 2019 at 15:46, on Zulip):

@oli clippy PR fails on macOS

Santiago Pastorino (Mar 01 2019 at 15:46, on Zulip):

unsure how is that setup and it may use nighly on macOS?

oli (Mar 01 2019 at 15:48, on Zulip):

Installing /Users/travis/.cargo/bin/rustup-toolchain-install-master
detecting the channel of the 17add2498f20bb5a638a65c53407c72ac11becf6 toolchain...
error: https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rustc-builds/17add2498f20bb5a638a65c53407c72ac11becf6/rust-src-nightly.tar.xz: timed out
error: toolchain 'master' is not installed

oli (Mar 01 2019 at 15:48, on Zulip):

I guess mac os isn't uploaded yet?

nikomatsakis (Mar 01 2019 at 16:38, on Zulip):

@Santiago Pastorino nice job btw -- I'm just sort of vaguely following along here =)

Santiago Pastorino (Mar 01 2019 at 18:02, on Zulip):

ahh no idea

Santiago Pastorino (Mar 01 2019 at 18:02, on Zulip):

@oli I guess that with your struct idea you want to remove Projection struct, ProjectionElem and those structures, right?

Santiago Pastorino (Mar 01 2019 at 18:50, on Zulip):

Santiago Pastorino nice job btw -- I'm just sort of vaguely following along here =)

:+1:, for now is just moving things around :)

Santiago Pastorino (Mar 01 2019 at 20:49, on Zulip):

oli I guess that with your struct idea you want to remove Projection struct, ProjectionElem and those structures, right?

@oli unsure what happen with this message ^^^, because it was delivered 2hs later than what I've sent it

Santiago Pastorino (Mar 01 2019 at 20:49, on Zulip):

just mentioning you in case you missed it :)

Santiago Pastorino (Mar 01 2019 at 20:51, on Zulip):

and also, btw

Santiago Pastorino (Mar 01 2019 at 20:51, on Zulip):

the idea is that base always have the base thing

Santiago Pastorino (Mar 01 2019 at 20:51, on Zulip):

but projection field is really the one that defines in what part of the path I am, right?

Santiago Pastorino (Mar 01 2019 at 20:51, on Zulip):

to be more concrete

Santiago Pastorino (Mar 01 2019 at 20:51, on Zulip):

for instance ...

Santiago Pastorino (Mar 01 2019 at 20:52, on Zulip):
-            mir::Place::Base(mir::PlaceBase::Local(ref local)) => {
+            mir::Place { base: mir::PlaceBase::Local(ref local), projection: mir::PlaceProjection::Base } => {
Santiago Pastorino (Mar 01 2019 at 20:52, on Zulip):

this is the right diff

Santiago Pastorino (Mar 01 2019 at 20:53, on Zulip):

so, a Place is really a base place when projection is PlaceProjection::Base

nikomatsakis (Mar 01 2019 at 20:55, on Zulip):

So, @Santiago Pastorino, in general I am still a bit confused by what oli's plan was about this (base, projection) tuple

nikomatsakis (Mar 01 2019 at 20:55, on Zulip):

I think maybe it was this

nikomatsakis (Mar 01 2019 at 20:56, on Zulip):

Right now, we have a setup like so:

P = <local variable>
    | <static>
    | *P (deref)
    | P.f
    | P[P]
nikomatsakis (Mar 01 2019 at 20:56, on Zulip):

does that notation make sense?

nikomatsakis (Mar 01 2019 at 20:56, on Zulip):

i.e., I'm using a grammar to describe the enum

Santiago Pastorino (Mar 01 2019 at 20:56, on Zulip):

yes

Santiago Pastorino (Mar 01 2019 at 20:56, on Zulip):

makes sense

nikomatsakis (Mar 01 2019 at 20:56, on Zulip):

So I think what @oli was proposing was

nikomatsakis (Mar 01 2019 at 20:56, on Zulip):

To change it to this

nikomatsakis (Mar 01 2019 at 20:57, on Zulip):
Place = (Base, Projection)
Base = <local varibable> | <static>
Projection = B | *Projection | Projection.F | Projection[Place]
nikomatsakis (Mar 01 2019 at 20:57, on Zulip):

where B is kind of a "special marker" to mean

nikomatsakis (Mar 01 2019 at 20:57, on Zulip):

"the base from this place"

Santiago Pastorino (Mar 01 2019 at 20:58, on Zulip):

yes

nikomatsakis (Mar 01 2019 at 20:58, on Zulip):

so now to represent *x.y you would not have

but rather

(x, *B.y)

nikomatsakis (Mar 01 2019 at 20:58, on Zulip):

that is, the pair of x and

Santiago Pastorino (Mar 01 2019 at 20:59, on Zulip):

yep

Santiago Pastorino (Mar 01 2019 at 20:59, on Zulip):

so, as I said, a base is a base when you have base = something and projection = that marker, right?

Santiago Pastorino (Mar 01 2019 at 21:00, on Zulip):

the marker being ProjectionPlace::Base

nikomatsakis (Mar 01 2019 at 21:00, on Zulip):

ah, right

nikomatsakis (Mar 01 2019 at 21:00, on Zulip):

yes

Santiago Pastorino (Mar 01 2019 at 21:00, on Zulip):

yes, that's more or less what I thought

nikomatsakis (Mar 01 2019 at 21:00, on Zulip):

so the path x is just (x, B)

nikomatsakis (Mar 01 2019 at 21:00, on Zulip):

ok

Santiago Pastorino (Mar 01 2019 at 21:00, on Zulip):

yes

Santiago Pastorino (Mar 01 2019 at 21:00, on Zulip):

makes sense

nikomatsakis (Mar 01 2019 at 21:00, on Zulip):

Yeah, it makes some sense

nikomatsakis (Mar 01 2019 at 21:01, on Zulip):

a lot of algorithms want "easy access" to the base

nikomatsakis (Mar 01 2019 at 21:01, on Zulip):

and this structure gives it to them

nikomatsakis (Mar 01 2019 at 21:01, on Zulip):

it's also "closer" to the (base, [projection]) setup we were shooting for

nikomatsakis (Mar 01 2019 at 21:01, on Zulip):

So does that unblock you, or is your question something more involved? :)

Santiago Pastorino (Mar 01 2019 at 21:28, on Zulip):

this is great

Santiago Pastorino (Mar 01 2019 at 21:28, on Zulip):

:)

Santiago Pastorino (Mar 01 2019 at 21:29, on Zulip):

starting to work on a change like this and figuring it out at the end that something is wrong would be very bad :)

Santiago Pastorino (Mar 01 2019 at 21:29, on Zulip):

oli I guess that with your struct idea you want to remove Projection struct, ProjectionElem and those structures, right?

@nikomatsakis this was my other assumption from that code

Santiago Pastorino (Mar 01 2019 at 21:29, on Zulip):

that there are things that are not needed anymore

nikomatsakis (Mar 01 2019 at 21:29, on Zulip):

seems likely

Santiago Pastorino (Mar 01 2019 at 21:30, on Zulip):

just super hyper checking stuff because this will be a massive change that would be, again, very bad if I do the wrong thing :)

Santiago Pastorino (Mar 07 2019 at 15:11, on Zulip):

@oli when does the break everything wondow closes?

Santiago Pastorino (Mar 07 2019 at 15:11, on Zulip):

I mean, if I were landing the Place PR that changes the representation to be a struct

oli (Mar 07 2019 at 15:11, on Zulip):

No idea, but I wouldn't worry, it's just a week, so at worst it'll be delayed by a week

Santiago Pastorino (Mar 07 2019 at 15:11, on Zulip):

when is the last moment to do so?

oli (Mar 07 2019 at 15:12, on Zulip):

usually the 7 days before a beta cut

oli (Mar 07 2019 at 15:12, on Zulip):

so like 14 days before a new stable release or sth?

Santiago Pastorino (Mar 07 2019 at 15:12, on Zulip):

usually the 7 days before a beta cut

ahhh

Santiago Pastorino (Mar 07 2019 at 15:12, on Zulip):

I thought that after the release it was 1 week of break everything in the 6 weeks period

Santiago Pastorino (Mar 07 2019 at 15:12, on Zulip):

I see that is 1 week of do not break in the 6 weeks window :)

Santiago Pastorino (Mar 07 2019 at 15:12, on Zulip):

ok

oli (Mar 07 2019 at 15:12, on Zulip):

nope, other way around

oli (Mar 18 2019 at 15:04, on Zulip):

we now have our own stream in the form of #t-compiler/wg-mir-opt So any future discussion of place 2.0 should take place (heh) there

Santiago Pastorino (Mar 18 2019 at 18:37, on Zulip):

@oli I was very busy with conference organization so I haven't touch Place 2.0 for a while

Santiago Pastorino (Mar 18 2019 at 18:37, on Zulip):

was just started again to do the struct thing we have pending :)

oli (Mar 19 2019 at 08:20, on Zulip):

no pressure!

Last update: Nov 20 2019 at 02:35UTC