Stream: t-compiler/wg-mir-opt

Topic: MutationUse and NonMutatingUse in visitor


Santiago Pastorino (Jun 07 2019 at 19:34, on Zulip):

@oli as we were discussing

Santiago Pastorino (Jun 07 2019 at 19:36, on Zulip):

the context thing that happens here https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/mir/analyze.rs is kind of weird to me

Santiago Pastorino (Jun 07 2019 at 19:36, on Zulip):

look at https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/mir/analyze.rs#L163-L166

Santiago Pastorino (Jun 07 2019 at 19:36, on Zulip):

and then

Santiago Pastorino (Jun 07 2019 at 19:36, on Zulip):

https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/mir/analyze.rs#L201-L205

Santiago Pastorino (Jun 07 2019 at 19:36, on Zulip):

the thing is going to match at most the first time and never again

Santiago Pastorino (Jun 07 2019 at 19:37, on Zulip):

if that's the intention I guess at least that shouldn't be inside the loop

Santiago Pastorino (Jun 07 2019 at 19:37, on Zulip):

I've changed that code to make iterative, maybe I was the one that the wrong thing :P

Santiago Pastorino (Jun 07 2019 at 19:38, on Zulip):

well, not the first time, is true that there's a continue

Santiago Pastorino (Jun 07 2019 at 19:38, on Zulip):

hmm the code is a bit weird to me

Santiago Pastorino (Jun 07 2019 at 19:39, on Zulip):

also, I'm just seeing this https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/mir/analyze.rs#L192-L199 too

Santiago Pastorino (Jun 07 2019 at 19:39, on Zulip):

so inside the iteration we have a recursion

Santiago Pastorino (Jun 07 2019 at 19:43, on Zulip):

first, I'm removing this https://github.com/rust-lang/rust/blob/master/src/librustc_codegen_ssa/mir/analyze.rs#L192-L199

Santiago Pastorino (Jun 07 2019 at 20:10, on Zulip):

just opened a PR https://github.com/rust-lang/rust/pull/61633

oli (Jun 07 2019 at 21:02, on Zulip):

hmm... is that really the same? The visit_place recursion essentially restarts the loop, right?

oli (Jun 07 2019 at 21:02, on Zulip):

after your change it continues the loop

Santiago Pastorino (Jun 07 2019 at 21:04, on Zulip):

yep but it also has a return

Santiago Pastorino (Jun 07 2019 at 21:04, on Zulip):

so it's the same

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

it's calling the method again with base but having a return on it, so current recursion will stop and a new one starting with the next element would be created

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

which is the same as just following to the next step of the recursion

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

anyway, that's a thing in master

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

not related to the problem we were trying to fix in the Place PR

oli (Jun 07 2019 at 21:08, on Zulip):

I'm not so sure, you're now using proj.base, so if you had a -> b -> deref -> c it used to be b, deref, recurse, b, a, but now you changed it to b, deref, c, a

Santiago Pastorino (Jun 07 2019 at 21:12, on Zulip):

confused

Santiago Pastorino (Jun 07 2019 at 21:12, on Zulip):

did you mean in the first example c, deref, recurse, b, a?

oli (Jun 07 2019 at 21:13, on Zulip):

no that's the recursive data structure: c(deref(b(a)))

oli (Jun 07 2019 at 21:13, on Zulip):

which the iterator turns into a, [b, deref, c]

Santiago Pastorino (Jun 07 2019 at 21:13, on Zulip):

yes

oli (Jun 07 2019 at 21:13, on Zulip):

so when we are at the deref, proj.base is b(a)

Santiago Pastorino (Jun 07 2019 at 21:14, on Zulip):

so ... it seems like there may be some missing tests then

Santiago Pastorino (Jun 07 2019 at 21:14, on Zulip):

maybe would be wise to check what the code did before my recurse to iterate change

Santiago Pastorino (Jun 07 2019 at 21:15, on Zulip):

because with this PR and without this PR tests are ok

oli (Jun 07 2019 at 21:19, on Zulip):

well... these code changes would only show up in llvm IR I presume

oli (Jun 07 2019 at 21:19, on Zulip):

and probably just end up being less ideal code

Santiago Pastorino (Jun 07 2019 at 21:22, on Zulip):

probably worth checking what did the code do before the iterate thing showed up then :)

Santiago Pastorino (Jun 07 2019 at 21:22, on Zulip):

trying to do that

Santiago Pastorino (Jun 07 2019 at 21:22, on Zulip):

https://github.com/rust-lang/rust/blob/01cd907e3beef1c9d261c11c3d7171aa0f3cd54c/src/librustc_codegen_ssa/mir/analyze.rs

oli (Jun 07 2019 at 21:25, on Zulip):

hmm, you're right, both recursions should behave the same

Santiago Pastorino (Jun 07 2019 at 21:26, on Zulip):

the old code was going straight from c to a

Santiago Pastorino (Jun 07 2019 at 21:26, on Zulip):

which is what the code does after my PR

Santiago Pastorino (Jun 07 2019 at 21:26, on Zulip):

the thing that master is doing right now, I have no clue :P

Santiago Pastorino (Jun 07 2019 at 21:26, on Zulip):

really confusing

oli (Jun 07 2019 at 21:27, on Zulip):

yea, the old code went from the outermost projection (so the last one in iteration) to the innermost (so to the first one)

oli (Jun 07 2019 at 21:28, on Zulip):

and it had early aborts for zsts

Santiago Pastorino (Jun 07 2019 at 21:28, on Zulip):

:+1:

Santiago Pastorino (Jun 07 2019 at 21:28, on Zulip):

this is what happens now with the PR because we even visit base last https://github.com/rust-lang/rust/pull/61633/files#diff-e682b38b565035a82f29560c3b79144bR201

oli (Jun 07 2019 at 21:28, on Zulip):

hmm... maybe we should just replicate this recursion until we have slices and can reverse iterate?

oli (Jun 07 2019 at 21:28, on Zulip):

base was always visited last

oli (Jun 07 2019 at 21:28, on Zulip):

if at all

Santiago Pastorino (Jun 07 2019 at 21:28, on Zulip):

yep

Santiago Pastorino (Jun 07 2019 at 21:28, on Zulip):

what do you mean by replicate the recursion?

Santiago Pastorino (Jun 07 2019 at 21:29, on Zulip):

to apply the PR?

Santiago Pastorino (Jun 07 2019 at 21:29, on Zulip):

or is it something wrong in it and I'm not seeing it?

oli (Jun 07 2019 at 21:29, on Zulip):

Just create custom functions that do exactly what the old code did

oli (Jun 07 2019 at 21:29, on Zulip):

but have base and projection as separate arguments

oli (Jun 07 2019 at 21:30, on Zulip):

hmm oh wait, we don't have that yet -.-

oli (Jun 07 2019 at 21:30, on Zulip):

I'm mixing up the PRs

oli (Jun 07 2019 at 21:30, on Zulip):

so yea, we should revert the iterate change for that function, it silently changed behaviour and I'm not even sure how to properly test for it

Santiago Pastorino (Jun 07 2019 at 21:30, on Zulip):

talking about this one https://github.com/rust-lang/rust/pull/61633

oli (Jun 07 2019 at 21:31, on Zulip):

oh yea, that transformation is wrong, but the code is wrong to begin with

oli (Jun 07 2019 at 21:31, on Zulip):

we can't do this in the recursion iteration way really

oli (Jun 07 2019 at 21:31, on Zulip):

eh

oli (Jun 07 2019 at 21:31, on Zulip):

we need to start with the outermost so we can early abort

oli (Jun 07 2019 at 21:31, on Zulip):

and do special things for immediates and derefs

oli (Jun 07 2019 at 21:32, on Zulip):

once we have slices, then we can replicate the current behaviour by recursing backwards

Santiago Pastorino (Jun 07 2019 at 21:33, on Zulip):

hehe, everytime I come back to this I get confused

Santiago Pastorino (Jun 07 2019 at 21:33, on Zulip):

iterate goes backwards

Santiago Pastorino (Jun 07 2019 at 21:33, on Zulip):

right?

Santiago Pastorino (Jun 07 2019 at 21:33, on Zulip):

well need to re-check the code to be honest I always forget

oli (Jun 07 2019 at 21:33, on Zulip):

iterate goes a.b.c in b -> c and a can come howeveryou want

oli (Jun 07 2019 at 21:34, on Zulip):

recursion goes c -> b -> a

Santiago Pastorino (Jun 07 2019 at 21:34, on Zulip):

yes yes

Santiago Pastorino (Jun 07 2019 at 21:34, on Zulip):

so it's wrong :)

Santiago Pastorino (Jun 07 2019 at 22:17, on Zulip):

https://github.com/rust-lang/rust/pull/61641

Santiago Pastorino (Jun 07 2019 at 22:17, on Zulip):

@oli ^^^

Last update: Nov 17 2019 at 08:25UTC