Stream: t-compiler/wg-mir-opt

Topic: non recursive Place construction


Santiago Pastorino (Sep 13 2019 at 21:27, on Zulip):

I was talking to @oli about avoiding recursion when constructing place on mir build

Santiago Pastorino (Sep 13 2019 at 21:28, on Zulip):

@nikomatsakis have some thoughts about it

Santiago Pastorino (Sep 16 2019 at 15:36, on Zulip):

@nikomatsakis told me some days ago ...

Santiago Pastorino (Sep 16 2019 at 15:36, on Zulip):
nikomatsakis: I just think working recursively is more natural
nikomatsakis: I think what I had in mind was that we returned not a Place
nikomatsakis: but rather a PlaceBuilder
nikomatsakis: that accumulates into a Vec
nikomatsakis: and then has some kind of into_place method that does the interning
Santiago Pastorino (Sep 16 2019 at 15:37, on Zulip):

/cc @oli

oli (Sep 16 2019 at 15:49, on Zulip):

as long as we don't end up with actual recursion... I don't care much about the API. We have stack overflows in degenerate usercode that can only be solved by moving to iteration

Santiago Pastorino (Sep 17 2019 at 18:10, on Zulip):

@nikomatsakis I was talking with @oli about making this part iterative and we were wondering what to do with https://github.com/rust-lang/rust/blob/f504e37dc64b2f00291f53517fe9cb2329d00fb8/src/librustc_mir/build/expr/as_place.rs#L55-L61

Santiago Pastorino (Sep 17 2019 at 18:10, on Zulip):

I guess this is why you've said that the recursion is more natural?

Santiago Pastorino (Sep 17 2019 at 18:10, on Zulip):

the rest of the cases are pretty straightforward

Santiago Pastorino (Sep 17 2019 at 18:11, on Zulip):

what I'm going to do, is keep recursion there and pass a vec around which I keep filling

Santiago Pastorino (Sep 17 2019 at 18:12, on Zulip):

you were mentioning a PlaceBuilder unsure what idea exactly you had in mind and if that could solve the in_scope problem

Santiago Pastorino (Sep 19 2019 at 13:37, on Zulip):

@nikomatsakis I was talking with @oli about this

Santiago Pastorino (Sep 19 2019 at 13:38, on Zulip):

I've done a quick and dirty try on making things iterate there

Santiago Pastorino (Sep 19 2019 at 13:38, on Zulip):

this https://github.com/rust-lang/rust/compare/master...spastorino:make-place-build-iterative?w=1 is what I have

Santiago Pastorino (Sep 19 2019 at 13:38, on Zulip):

as you can see, it's bad :P

Santiago Pastorino (Sep 19 2019 at 13:38, on Zulip):

mainly we need to get somethings upfront in a lot of cases so we need to recurse anyway

Santiago Pastorino (Sep 19 2019 at 13:39, on Zulip):

like for instance https://github.com/rust-lang/rust/compare/master...spastorino:make-place-build-iterative?w=1#diff-cfb4737a5d62dc17cfbc2d1f73ffff35R90

Santiago Pastorino (Sep 19 2019 at 13:41, on Zulip):

I guess maybe this are the kind of things we were referring to when you've said that recursion is more natural in this case?

Santiago Pastorino (Sep 19 2019 at 13:42, on Zulip):

@nikomatsakis when you have some time to check this out, would be nice to hear from you on what to do here :)

oli (Sep 19 2019 at 13:45, on Zulip):

I should've probably read nikos chat log better... We can return a builder (glorified (PlaceBase, Vec<PlaceElem>)) instead of a Place and give that a finish method for getting a Place out of it

oli (Sep 19 2019 at 13:46, on Zulip):

then, when all you do is push an element and return, all we need to do is push to the builder

Santiago Pastorino (Sep 19 2019 at 13:47, on Zulip):

yeah and most of the logic we have right now is done in the "finish" fn or as Niko said "into_place"

Santiago Pastorino (Sep 19 2019 at 13:48, on Zulip):

to be more clear, I meant, most of what we have right now in expr_as_place moves to finish or into_place or whatever we call it

nikomatsakis (Sep 19 2019 at 20:21, on Zulip):

as long as we don't end up with actual recursion... I don't care much about the API. We have stack overflows in degenerate usercode that can only be solved by moving to iteration

hmm, I wasn't trying to solve that problem, which I thought we'd solve by just growing the stack

nikomatsakis (Sep 19 2019 at 20:21, on Zulip):

this is what I meant by "recursion is just more natural"

nikomatsakis (Sep 19 2019 at 20:21, on Zulip):

I should've probably read nikos chat log better... We can return a builder (glorified (PlaceBase, Vec<PlaceElem>)) instead of a Place and give that a finish method for getting a Place out of it

this is what I was proposing, yes

Santiago Pastorino (Sep 19 2019 at 20:23, on Zulip):

to be more clear, I meant, most of what we have right now in expr_as_place moves to finish or into_place or whatever we call it

basically this ^^^?

Santiago Pastorino (Sep 19 2019 at 20:24, on Zulip):

I meant, now there we just construct the builder and move the logic on expr_as_place to the finish?, that's what you meant?

oli (Sep 20 2019 at 06:08, on Zulip):

all the logic will still happen where it does, but it won't build an actual Place, it will delay that until a Place is needed

oli (Sep 20 2019 at 06:10, on Zulip):

essentially:

  1. change the return type to the builder
  2. change all return sites to emit a builder (by just doing what's being done now: .to_vec() the projection and clone the base)
  3. Add a .finish() to all call sites
  4. get rid of unnecessary .finish().to_builder() calls by pushing to the projections Vec and returning the builder unfinished
Santiago Pastorino (Sep 30 2019 at 16:12, on Zulip):

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

nikomatsakis (Sep 30 2019 at 19:27, on Zulip):

nice, @Santiago Pastorino :)

Santiago Pastorino (Sep 30 2019 at 20:24, on Zulip):

@nikomatsakis @oli what I've said about removing this functions https://github.com/rust-lang/rust/blob/40a5331753e44ef8ef2d0b68fe6ee35234ab82ad/src/librustc/mir/mod.rs#L1862-L1894 from Place doesn't seem to be a good idea, unsure if you have some thoughts about it

Santiago Pastorino (Sep 30 2019 at 20:24, on Zulip):

there are some cases outside as_place where it's clearly not convenient

Santiago Pastorino (Sep 30 2019 at 20:24, on Zulip):

we may end converting a Place to a PlaceBuilder to convert it back to Place, so things will be worser

Santiago Pastorino (Sep 30 2019 at 20:25, on Zulip):

but I was hoping there were cases where those calls could be converted to use the builder

Santiago Pastorino (Sep 30 2019 at 20:25, on Zulip):

doesn't seem like that though

Santiago Pastorino (Sep 30 2019 at 20:35, on Zulip):

other than changing this as_place call with a call to as_place_builder and then call PlaceBuilder's field method followed by an as_place call here

Santiago Pastorino (Sep 30 2019 at 20:35, on Zulip):

unsure if it worth doing that

Santiago Pastorino (Sep 30 2019 at 20:39, on Zulip):

it's inside a closure, so I'd need to clone it all the times that's called anyway

Santiago Pastorino (Sep 30 2019 at 20:39, on Zulip):

I guess it ends being more or less equivalent

Santiago Pastorino (Oct 01 2019 at 14:00, on Zulip):

so ... we have perf results for https://github.com/rust-lang/rust/pull/64922

Santiago Pastorino (Oct 01 2019 at 14:00, on Zulip):

it doesn't seem to be showing useful wins

Santiago Pastorino (Oct 01 2019 at 14:00, on Zulip):

/cc @oli @nikomatsakis others that want to check this out ^^^

Santiago Pastorino (Oct 02 2019 at 12:49, on Zulip):

@oli was talking with @nikomatsakis in private and he thinks that we should merge this, he wasn't expecting huge wins neither

Santiago Pastorino (Oct 02 2019 at 12:49, on Zulip):

I've said that I was expecting a bit more but still ...

oli (Oct 02 2019 at 12:51, on Zulip):

sgtm

Last update: Nov 17 2019 at 07:35UTC