Stream: t-compiler/wg-mir-opt

Topic: using iterate on visit_place


Santiago Pastorino (May 31 2019 at 20:38, on Zulip):

I was discussing with @oli earlier about using Place::iterate to implement visit_place and have some better API there

Santiago Pastorino (May 31 2019 at 20:40, on Zulip):

right now, visit_place does https://github.com/rust-lang/rust/blob/master/src/librustc/mir/visit.rs#L686

Santiago Pastorino (May 31 2019 at 20:40, on Zulip):

and the visit_projection calls visit_place again passing base https://github.com/rust-lang/rust/blob/master/src/librustc/mir/visit.rs#L701 so you end in this way recursing and traversing the full thing

Santiago Pastorino (May 31 2019 at 20:41, on Zulip):

I wanted to propose an API that's going to be better for Place 2.0 and a bit better IMO

Santiago Pastorino (May 31 2019 at 20:42, on Zulip):

basically we can have visit_place iterating over the projections backwards and calling visit_projection on each and then calling visit_place_base on the base

Santiago Pastorino (May 31 2019 at 20:42, on Zulip):

to do that you call iterate have the base and the proj iterator

Santiago Pastorino (May 31 2019 at 20:42, on Zulip):

something like ...

Santiago Pastorino (May 31 2019 at 20:43, on Zulip):
place.iterate(|place_base, place_projection| {
    for proj in place_projection {
        self.visit_projection(proj, context, location);
    }

    self.visit_place_base(place_base, context, location);
})
Santiago Pastorino (May 31 2019 at 20:44, on Zulip):

so implementors can just implement visit_projection if they need to change that behavior, can implement visit_place_base and rely on the fact that the default visit_place impl will traverse everything or can just implement the full thing if they need to change everything

Santiago Pastorino (May 31 2019 at 20:46, on Zulip):

the problem I'm having is that iterate takes a shared reference and this visitors definitions are under a macro that uses mutability to define the mutable or immutable visitor

Santiago Pastorino (May 31 2019 at 20:47, on Zulip):

so in particular that proj you see in the code is an immutable thing

Santiago Pastorino (May 31 2019 at 20:47, on Zulip):

but visit_projection signature is:

Santiago Pastorino (May 31 2019 at 20:48, on Zulip):
fn visit_projection(&mut self,
                    place_projection: & $($mutability)? Projection<'tcx>,
                    context: PlaceContext,
                    location: Location) {
Santiago Pastorino (May 31 2019 at 20:49, on Zulip):

a couple of questions:

Santiago Pastorino (May 31 2019 at 20:49, on Zulip):

1. do you think that worth changing the API?

Santiago Pastorino (May 31 2019 at 20:50, on Zulip):

2. in that case, I guess the best thing I can do is define iterate and iterate_mut, right?, any other better approach?

nikomatsakis (May 31 2019 at 21:22, on Zulip):

1. do you think that worth changing the API?

do we know any places that will want to visit the "base" and "projection" cases?

nikomatsakis (May 31 2019 at 21:22, on Zulip):

versus overloading visit_place as a whole

Santiago Pastorino (May 31 2019 at 21:24, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/blob/master/src/librustc_mir/monomorphize/collector.rs#L663-L676

Santiago Pastorino (May 31 2019 at 21:25, on Zulip):

I guess this is only about overriding base behavior and doing the default thing afterwards

nikomatsakis (May 31 2019 at 21:25, on Zulip):

I feel like that case would work equally well

nikomatsakis (May 31 2019 at 21:25, on Zulip):

if you only had a hook on visit_place

nikomatsakis (May 31 2019 at 21:25, on Zulip):

i.e., there is one base, so it could just be

nikomatsakis (May 31 2019 at 21:26, on Zulip):
fn visit_place(place: &mir::Place) {
  match place.base { ... }
}
nikomatsakis (May 31 2019 at 21:26, on Zulip):

(especially in MIR 2.0 where this is very efficient)

nikomatsakis (May 31 2019 at 21:26, on Zulip):

I think a more compelling case might be something like visit_local

nikomatsakis (May 31 2019 at 21:26, on Zulip):

which I...think we also have?

nikomatsakis (May 31 2019 at 21:26, on Zulip):

i.e., we should visit the base recursively

Santiago Pastorino (May 31 2019 at 21:26, on Zulip):

well, on MIR 2.0 would be different

nikomatsakis (May 31 2019 at 21:26, on Zulip):

but that's the easy case

nikomatsakis (May 31 2019 at 21:27, on Zulip):

ah let's not forget

Santiago Pastorino (May 31 2019 at 21:27, on Zulip):

iterate probably won't exist anymore

nikomatsakis (May 31 2019 at 21:27, on Zulip):

foo.bar[baz]

nikomatsakis (May 31 2019 at 21:27, on Zulip):

where baz would itself be a place (that is part of a projection)

Santiago Pastorino (May 31 2019 at 21:27, on Zulip):

iterate would just be (place.base, place.projection)

Santiago Pastorino (May 31 2019 at 21:27, on Zulip):

and hence could be removed

Santiago Pastorino (May 31 2019 at 21:27, on Zulip):

but iterate is a handy method to move towards Place 2.0

Santiago Pastorino (May 31 2019 at 21:28, on Zulip):

I can just change the impl of it and done

Santiago Pastorino (May 31 2019 at 21:29, on Zulip):

this https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/inline.rs#L690-L709 is also only about base

Santiago Pastorino (May 31 2019 at 21:31, on Zulip):

anyway, I guess my main question then is the opposite, why there's a default implementation if it's not very useful and we always override it?

Santiago Pastorino (May 31 2019 at 21:32, on Zulip):

@nikomatsakis I'm trying to see if it's possible to do a default impl that is more convenient

nikomatsakis (May 31 2019 at 21:32, on Zulip):

anyway, I guess my main question then is the opposite, why there's a default implementation if it's not very useful and we always override it?

why do you say that?

nikomatsakis (May 31 2019 at 21:33, on Zulip):

there are definitely cases where we don't override it

nikomatsakis (May 31 2019 at 21:33, on Zulip):

it seems like the current one does the same thing as what you are proposing, basically

nikomatsakis (May 31 2019 at 21:33, on Zulip):

that is, it recurses into the content

Santiago Pastorino (May 31 2019 at 21:34, on Zulip):

yes it does

nikomatsakis (May 31 2019 at 21:34, on Zulip):

in any case, looking at a few more examples and thinking about it

Santiago Pastorino (May 31 2019 at 21:34, on Zulip):

the problem is the it traverses the thing using recursion

nikomatsakis (May 31 2019 at 21:34, on Zulip):

I think you probably do want to recurse -- at least, I see a number of MIR visitors that only override visit_local

Santiago Pastorino (May 31 2019 at 21:34, on Zulip):

and I'm changing the structure to not be recursive

nikomatsakis (May 31 2019 at 21:35, on Zulip):

and a local can be inside a place

nikomatsakis (May 31 2019 at 21:35, on Zulip):

so we do need the default impl of visit_place to recurse and handle those cases

Santiago Pastorino (May 31 2019 at 21:35, on Zulip):

basically this relies on the implementor calling super_place to have the recursion

Santiago Pastorino (May 31 2019 at 21:35, on Zulip):

and that's a thing I don't like that much

nikomatsakis (May 31 2019 at 21:35, on Zulip):

basically this relies on the implementor calling super_place to have the recursion

what do you mean by "this"

nikomatsakis (May 31 2019 at 21:36, on Zulip):

I don't really understand your critique of the current structure, which I think is pretty natural given that place is a tree right now, but I agree it's not the obvious structure you want if place is not a tree

nikomatsakis (May 31 2019 at 21:36, on Zulip):

I think the structure you're proposing seems right to me

nikomatsakis (May 31 2019 at 21:36, on Zulip):

but it will need an iterator_mut method presumably

nikomatsakis (May 31 2019 at 21:36, on Zulip):

kind of annoying

Santiago Pastorino (May 31 2019 at 21:37, on Zulip):

I don't really understand your critique of the current structure, which I think is pretty natural given that place is a tree right now, but I agree it's not the obvious structure you want if place is not a tree

yeah, that's the thing

Santiago Pastorino (May 31 2019 at 21:37, on Zulip):

the current structure is right for the recursive Place

Santiago Pastorino (May 31 2019 at 21:37, on Zulip):

for instance https://github.com/rust-lang/rust/blob/master/src/librustc_mir/transform/inline.rs#L707

Santiago Pastorino (May 31 2019 at 21:37, on Zulip):

that's problematic when you're structure is not recursive

Santiago Pastorino (May 31 2019 at 21:38, on Zulip):

basically I need to avoid that call and roll all the Place traversing there

Santiago Pastorino (May 31 2019 at 21:38, on Zulip):

that's why the default implementation is not helping that much

nikomatsakis (May 31 2019 at 21:39, on Zulip):

ok, so maybe I see what you are saying: the implementors of today's visitor are expecting a recursive structure

nikomatsakis (May 31 2019 at 21:39, on Zulip):

and have to be adapted to the new API?

nikomatsakis (May 31 2019 at 21:39, on Zulip):

(if so, I argee)

Santiago Pastorino (May 31 2019 at 21:39, on Zulip):

:+1:

Santiago Pastorino (May 31 2019 at 21:39, on Zulip):

and also

Santiago Pastorino (May 31 2019 at 21:39, on Zulip):

but it will need an iterator_mut method presumably

this structure I'm talking about it's probable a mid step in my PR

nikomatsakis (May 31 2019 at 21:40, on Zulip):

(it might even want a different name)

nikomatsakis (May 31 2019 at 21:40, on Zulip):

just to make sure you look at each impl

Santiago Pastorino (May 31 2019 at 21:40, on Zulip):

it will probably not be present after the final implementation is ready with projections being an slice

Santiago Pastorino (May 31 2019 at 21:40, on Zulip):

once projection is a slice

nikomatsakis (May 31 2019 at 21:41, on Zulip):

so that's an interesting thing

nikomatsakis (May 31 2019 at 21:41, on Zulip):

I forget what the final API is going to be here but

Santiago Pastorino (May 31 2019 at 21:41, on Zulip):

you can just do place.base and for proj in place.projection

nikomatsakis (May 31 2019 at 21:41, on Zulip):

visiting "mutably" may not be so easy

nikomatsakis (May 31 2019 at 21:41, on Zulip):

if the place is to have projections be an interned slice

nikomatsakis (May 31 2019 at 21:41, on Zulip):

since that will not be &mut

Wesley Wiser (May 31 2019 at 21:42, on Zulip):

(Is there a recent-ish write up about what Place 2.0 is going to be? I'm not sure I know enough to even form an opinion.)

Santiago Pastorino (May 31 2019 at 21:42, on Zulip):

yeah, we would probably want to not have visit_projection anymore

nikomatsakis (May 31 2019 at 21:42, on Zulip):

there aren't very many mut visitors I don't think

nikomatsakis (May 31 2019 at 21:42, on Zulip):

the one in NLL will be a bit of a difficulty

nikomatsakis (May 31 2019 at 21:42, on Zulip):

(it rewrites types, and types can appear in projections etc)

Santiago Pastorino (May 31 2019 at 21:43, on Zulip):

@Wesley Wiser I think it's here https://paper.dropbox.com/doc/Place-2.0-current-PR-status--AeI5IB7l~bBywohy7t9iTW03Ag-vmbnFv8VkCEuL57QfWWMH

Santiago Pastorino (May 31 2019 at 21:44, on Zulip):

yeah, we would probably want to not have visit_projection anymore

I mean, we can just have visit_place and that won't be a problem, but unsure I see what you're saying

Santiago Pastorino (May 31 2019 at 21:44, on Zulip):

(it rewrites types, and types can appear in projections etc)

ahh we would need to have write access there, so ... ???

nikomatsakis (May 31 2019 at 21:45, on Zulip):

yes, the mut-visitor structure is just kind of problematic here

nikomatsakis (May 31 2019 at 21:45, on Zulip):

it seems like a lot of the mut-visitors that exist override visit_local

nikomatsakis (May 31 2019 at 21:45, on Zulip):

(which can also appear in projections)

nikomatsakis (May 31 2019 at 21:46, on Zulip):

I don't know what's the best solution here

nikomatsakis (May 31 2019 at 21:46, on Zulip):

but it seems to me that the MutVisitor will have to offer a more limited API

nikomatsakis (May 31 2019 at 21:46, on Zulip):

one can update a Place but to modify things within a place you will have to use a "folder" and re-intern the results

nikomatsakis (May 31 2019 at 21:47, on Zulip):

on the one hand, the current MutVisitor is very nicely efficient (modifying in place etc)..

nikomatsakis (May 31 2019 at 21:47, on Zulip):

...but there is also a good case for making it more "folder-like"

nikomatsakis (May 31 2019 at 21:48, on Zulip):

(...and actually it probably travereses a lot of IR that will never be modified, I wonder if LLVM can strip that stuff out)

nikomatsakis (May 31 2019 at 21:48, on Zulip):

by folder-like I mean having an interface like fn fold_local(Local) -> Local

nikomatsakis (May 31 2019 at 21:48, on Zulip):

instead of visit_local(&mut Local)

nikomatsakis (May 31 2019 at 21:48, on Zulip):

I guess it'd be worth looking at what the existing mut visitors do exactly

Santiago Pastorino (May 31 2019 at 21:48, on Zulip):

ok :)

nikomatsakis (May 31 2019 at 21:49, on Zulip):

my guess is we'll wind up with a more limited MutVisitor API

nikomatsakis (May 31 2019 at 21:49, on Zulip):

one that includes things like visit_statement

Santiago Pastorino (May 31 2019 at 21:49, on Zulip):

:+1:

nikomatsakis (May 31 2019 at 21:49, on Zulip):

but not the really detailed things like visit_local

nikomatsakis (May 31 2019 at 21:49, on Zulip):

or visit_ty

nikomatsakis (May 31 2019 at 21:50, on Zulip):

those we would instead want to be more "folder-like"

nikomatsakis (May 31 2019 at 21:50, on Zulip):

so that they can apply to projections too

Santiago Pastorino (May 31 2019 at 21:50, on Zulip):

:+1:

Santiago Pastorino (May 31 2019 at 21:50, on Zulip):

so back a bit about what I was saying, do you think I should do the iterate_mut thing then?

Santiago Pastorino (May 31 2019 at 21:50, on Zulip):

I meant, meanwhile

Santiago Pastorino (May 31 2019 at 21:50, on Zulip):

and probably as a mid step

nikomatsakis (May 31 2019 at 21:51, on Zulip):

I guess it's a decent mid-way step

Santiago Pastorino (May 31 2019 at 21:51, on Zulip):

:+1:

nikomatsakis (May 31 2019 at 21:52, on Zulip):

you definitely don't want to do what I'm suggesting above all together

Santiago Pastorino (May 31 2019 at 21:52, on Zulip):

hehe no no :)

Santiago Pastorino (May 31 2019 at 21:52, on Zulip):

but also, I'm not even adding projection as a slice yet

Santiago Pastorino (May 31 2019 at 21:53, on Zulip):

we defined a previous step which is migrating Place to a struct but still having projection be recursive

Santiago Pastorino (May 31 2019 at 21:53, on Zulip):

@nikomatsakis https://github.com/rust-lang/rust/pull/60913/files#diff-f0577ac900ffbd36d3bb3421a928cbbdR1909

Santiago Pastorino (Jun 05 2019 at 18:19, on Zulip):

@oli @nikomatsakis just had a couple of minutes and went ahead with what I had on my head about visit_place

Santiago Pastorino (Jun 05 2019 at 18:19, on Zulip):

just opened as WIP this https://github.com/rust-lang/rust/pull/61554

Santiago Pastorino (Jun 05 2019 at 18:19, on Zulip):

I've explained the reasoning there, let me know if it's clear and what do you think about that

Santiago Pastorino (Jun 05 2019 at 18:20, on Zulip):

IMO, the code this way will be a bit more clear

Santiago Pastorino (Jun 05 2019 at 18:20, on Zulip):

I can continue with the rest of the things that need to be changed to see if makes sense but wanted to check with you both first

Santiago Pastorino (Jun 05 2019 at 19:32, on Zulip):

@nikomatsakis @oli after reviewing the rest of the visit_place impls there are no more changes than what's on the PR

Santiago Pastorino (Jun 05 2019 at 19:32, on Zulip):

so unsure if it worth, anyway give it a look

Santiago Pastorino (Jun 05 2019 at 19:32, on Zulip):

I'm going to open another PR to change some of the visit_place impls to use iterate

Santiago Pastorino (Jun 05 2019 at 21:00, on Zulip):

@oli this is the other PR https://github.com/rust-lang/rust/pull/61559

Santiago Pastorino (Jun 05 2019 at 21:02, on Zulip):

if you don't like the first PR, maybe we can have visit_place_base anyway so we can get rid of https://github.com/rust-lang/rust/pull/61559/files#diff-e682b38b565035a82f29560c3b79144bR208

Last update: Nov 17 2019 at 07:40UTC