Stream: t-compiler/help

Topic: MIR building question


tmandry (May 28 2019 at 23:20, on Zulip):

https://github.com/rust-lang/rust/blob/721268583759224d0f6476e0b8b196cc8afbdea0/src/librustc_mir/build/scope.rs#L693-L739
^ this call to invalidate_cache causes the unwind cache for every drop in an inner scope to be invalidated. Can someone explain why this is necessary?

tmandry (May 28 2019 at 23:22, on Zulip):

i.e. if we added drop(new) to the outer scope, presumably none of the drops that would already added to the inner scope would ever need to go through this new drop

tmandry (May 28 2019 at 23:23, on Zulip):

or are there cases where you would need to?

nagisa (May 28 2019 at 23:26, on Zulip):

cache here is for entry points, not for drops themselves.

tmandry (May 28 2019 at 23:28, on Zulip):

cc @eddyb

nagisa (May 28 2019 at 23:28, on Zulip):

For reference I wrote that comment and the caching code.

tmandry (May 28 2019 at 23:29, on Zulip):

@nagisa entry points to what? I'm specifically referring to this line where the drop cache is invalidated: https://github.com/rust-lang/rust/blob/721268583759224d0f6476e0b8b196cc8afbdea0/src/librustc_mir/build/scope.rs#L245

tmandry (May 28 2019 at 23:29, on Zulip):

@nagisa sorry, I'd already asked eddyb to help and they couldn't find the topic

tmandry (May 28 2019 at 23:30, on Zulip):

@nagisa but your help is welcome :)

nagisa (May 28 2019 at 23:30, on Zulip):

So if we did not invalidate the cache for "middle scope", its branch would still be into the "old" inner outer scope which may potentially contain new drops.

nagisa (May 28 2019 at 23:31, on Zulip):

it is quite possible that the cache invalidation here is overzealous but it is also necessary at the granularity of invalidation that we currently have.

tmandry (May 28 2019 at 23:33, on Zulip):

do you mean the "old" outer scope? I don't think the middle scope would branch to the inner scope

nagisa (May 28 2019 at 23:33, on Zulip):

To answer the question about entry points: the cache maps scope -> bb if my memory serves me right and it is possible for a scope to gain new drops as the building of MIR goes on. So if a drop gets added into some scope that’s more inner the whole chain of scopes and their basic blocks must be rebuilt to eventually reference some basic block with the new drop included

nagisa (May 28 2019 at 23:36, on Zulip):

do you mean the "old" outer scope? I don't think the middle scope would branch to the inner scope

Yeah I meant outer scope.

tmandry (May 28 2019 at 23:37, on Zulip):

I think part of what's confusing in this conversation is that there are two caches, one on the scope level (cached_exits) and one on the per-drop level for unwinds

tmandry (May 28 2019 at 23:37, on Zulip):

I'm specifically asking about the unwind cache

tmandry (May 28 2019 at 23:39, on Zulip):

not sure if that was clear

nagisa (May 28 2019 at 23:39, on Zulip):

The explanation applies as much there as it does here.

nagisa (May 28 2019 at 23:40, on Zulip):

Like I said, it is likely that the invalidation implementation is somewhat overzealous, but it is only because it is not granular enough to know what exactly it needs to invalidate.

tmandry (May 28 2019 at 23:43, on Zulip):

Hmm okay, so just to make sure, this is for the situation where we add a new drop to an outer/middle scope, and we need to invalidate the cached unwind blocks of more "inner" scopes

nagisa (May 28 2019 at 23:44, on Zulip):

Well, entry points at certain scopes, but yeah.

nagisa (May 28 2019 at 23:46, on Zulip):

What gets cached is an entry point to a whole chain of drops until the the exit from the function. So everything more-inner than the added drop must be invalidated.

tmandry (May 28 2019 at 23:46, on Zulip):

ok I think this is where my understanding of the code was wrong

tmandry (May 28 2019 at 23:46, on Zulip):

I was assuming that we would only add the drop to the outer scope at this point, if the drop occurs after the inner scope has already ended

tmandry (May 28 2019 at 23:46, on Zulip):

but if the scope had already ended at this point in the MIR, presumably it wouldn't be on our stack of scopes

tmandry (May 28 2019 at 23:48, on Zulip):

I'm basically trying to imagine a situation where this actually occurs when building the MIR :)

nagisa (May 28 2019 at 23:49, on Zulip):

Inner-most scopes come and go, yes, but they can continue to add drops into scopes that are more outer than the innermost scope. For example let x = { let y = Droppable; Droppable } adds a drop to an inner scope as a part of an assignment to y and another one to a more outer scope as a part of assignment to x.

nagisa (May 28 2019 at 23:49, on Zulip):

even though both drops get added when the innermost scope is on the scope stack.

nagisa (May 28 2019 at 23:50, on Zulip):

(AFAIR anyway, it has been a long while since I dealt with that code).

nagisa (May 28 2019 at 23:50, on Zulip):

There are a number of complications when building match code for example.

nagisa (May 28 2019 at 23:51, on Zulip):

We used to have a case where we would build N identical drop chains for each match, which was the motivation for this code.

tmandry (May 28 2019 at 23:52, on Zulip):

ahh okay

tmandry (May 28 2019 at 23:52, on Zulip):

even though the match was considered all one scope

nagisa (May 28 2019 at 23:52, on Zulip):

https://github.com/rust-lang/rust/pull/34307 has some pretty images.

nagisa (May 28 2019 at 23:53, on Zulip):

(namely https://cloud.githubusercontent.com/assets/679122/16122708/ce0024d8-33f0-11e6-93c2-e1c44b910db2.png and https://cloud.githubusercontent.com/assets/679122/16122806/294fb16e-33f1-11e6-95f6-16c5438231af.png)

tmandry (May 28 2019 at 23:53, on Zulip):

that is quite a cleanup :D

tmandry (May 28 2019 at 23:53, on Zulip):

(no pun intended)

tmandry (May 28 2019 at 23:55, on Zulip):

Okay, I feel like I kind of understand this now. Thanks XD

nagisa (May 28 2019 at 23:57, on Zulip):

You’re welcome.

Last update: Nov 11 2019 at 23:10UTC