Stream: t-compiler/wg-mir-opt

Topic: removing unevaluated dead constants as defined per nll


Santiago Pastorino (Apr 06 2020 at 21:35, on Zulip):

I've talked with @oli some days ago and now with @nikomatsakis about removing unevaluated dead constants as defined per nll

Santiago Pastorino (Apr 06 2020 at 21:35, on Zulip):

I was wondering how to do so

nikomatsakis (Apr 06 2020 at 21:35, on Zulip):

Right so what I was saying is that:

Santiago Pastorino (Apr 06 2020 at 21:35, on Zulip):

cc @eddyb

nikomatsakis (Apr 06 2020 at 21:35, on Zulip):

and the rough idea here is that we want to find the constants reachable from that code and report errors that occur in their definitions

nikomatsakis (Apr 06 2020 at 21:35, on Zulip):

even if we might find later that some of that code is dead

nikomatsakis (Apr 06 2020 at 21:36, on Zulip):

(e.g., because of if false { ... })

nikomatsakis (Apr 06 2020 at 21:36, on Zulip):

at least that is my memory

nikomatsakis (Apr 06 2020 at 21:36, on Zulip):

but what I don't remember quite is how/when the constant error reporting actually happens

eddyb (Apr 06 2020 at 21:36, on Zulip):

wouldn't keeping all the constants outside of the basic blocks let them be removed freely from inside basic blocks?

Santiago Pastorino (Apr 06 2020 at 21:36, on Zulip):

btw this is about #67191

Santiago Pastorino (Apr 06 2020 at 21:37, on Zulip):

eddyb said:

wouldn't keeping all the constants outside of the basic blocks let them be removed freely from inside basic blocks?

but we want to remove the ones from this new vector I was adding

Santiago Pastorino (Apr 06 2020 at 21:37, on Zulip):

so we do not error on return something; refer_to_constant

eddyb (Apr 06 2020 at 21:38, on Zulip):

but don't we want to error on that?

eddyb (Apr 06 2020 at 21:38, on Zulip):

I'm slightly confused :)

Santiago Pastorino (Apr 06 2020 at 21:38, on Zulip):

to be honest at some point I was talking with @centril about this and I do not understand

Santiago Pastorino (Apr 06 2020 at 21:38, on Zulip):

but read somewhere that this was defined on a lang meeting

Santiago Pastorino (Apr 06 2020 at 21:39, on Zulip):

I don't understand why doing more work to report less errors is better but I guess consistency is what was considered more important

eddyb (Apr 06 2020 at 21:39, on Zulip):

if we want to keep only some consts, collect them just after the post-NLL simplifycfg or w/e

eddyb (Apr 06 2020 at 21:39, on Zulip):

before any optimizations that could remove consts in non-dead code

nikomatsakis (Apr 06 2020 at 21:41, on Zulip):

I think that is roughly what we want to do -- but I'm not 100% sure what you mean by "collect" in that sentence

eddyb (Apr 06 2020 at 21:42, on Zulip):

gather them into the separate Vec<ty::Const> that @Santiago Pastorino wants to add

Santiago Pastorino (Apr 06 2020 at 21:43, on Zulip):

@oli also told me

I guess we could make NLL fill in that array
but I don't like it
I'd rather do a post-NLL optimization pass that removes all things NLL decided are unreachable
(which... should be very easy, because it simply is everything that still exists at that point)
so DCE after NLL would also clean up those constants
nikomatsakis (Apr 06 2020 at 21:43, on Zulip):

yeah I just don't 100% know where that vector 'lives' -- anyway I'm basically asking questions that are quite ignorant from not skimming the code.

nikomatsakis (Apr 06 2020 at 21:43, on Zulip):

in my imagination there is some pass that walks the MIR and tries to evaluate constants it finds and reports errors

Santiago Pastorino (Apr 06 2020 at 21:43, on Zulip):

nikomatsakis said:

yeah I just don't 100% know where that vector 'lives' -- anyway I'm basically asking questions that are quite ignorant from not skimming the code.

the vector lives in mir body

nikomatsakis (Apr 06 2020 at 21:44, on Zulip):

I see, and we later iterate over this vector?

eddyb (Apr 06 2020 at 21:44, on Zulip):

yeah in codegen

Santiago Pastorino (Apr 06 2020 at 21:44, on Zulip):

yes

Santiago Pastorino (Apr 06 2020 at 21:44, on Zulip):

nikomatsakis said:

in my imagination there is some pass that walks the MIR and tries to evaluate constants it finds and reports errors

the problem with this is that before this new PR, that happened but only with live constants not with the ones unreachable

nikomatsakis (Apr 06 2020 at 21:44, on Zulip):

then yes this seems roughly right. I guess the fact that we have an &mut Body means we can basically either remove things from the vector or have it be empty and just populate it at the right time?

Santiago Pastorino (Apr 06 2020 at 21:45, on Zulip):

and we want to check the ones that are not reachable to report errors

nikomatsakis (Apr 06 2020 at 21:45, on Zulip):

well I think the idea was to report errors for live constants, but we just have to be sure which definition of "live" we want to use

Santiago Pastorino (Apr 06 2020 at 21:45, on Zulip):

ohh right, I didn't mean live in the nll sense of live :)

nikomatsakis (Apr 06 2020 at 21:46, on Zulip):

i.e., if we just walk the set of blocks reachable from the start block (at the time of borrow check, before doing any optimizations), then it's any constants referenced from within those blocks?

Santiago Pastorino (Apr 06 2020 at 21:46, on Zulip):

I meant stuff that is reachable and wasn't optimized away

nikomatsakis (Apr 06 2020 at 21:46, on Zulip):

alternatively, if we run DCE (which we maybe even already do, I can't remember) before doing any other optimizations, then it's just "all blocks"

Santiago Pastorino (Apr 06 2020 at 21:48, on Zulip):

nikomatsakis said:

then yes this seems roughly right. I guess the fact that we have an &mut Body means we can basically either remove things from the vector or have it be empty and just populate it at the right time?

sorry, didn't get what's roughly right?

nikomatsakis (Apr 06 2020 at 21:48, on Zulip):

what eddyb wrote here:

if we want to keep only some consts, collect them just after the post-NLL simplifycfg or w/e

Santiago Pastorino (Apr 06 2020 at 21:50, on Zulip):

:+1:

eddyb (Apr 06 2020 at 21:55, on Zulip):

you don't even need simplifycfg if you use an RPO traversal :P

eddyb (Apr 06 2020 at 21:56, on Zulip):

actually do we have a cheaper VecDeque+BitSet traversal that doesn't preserve dominator ordering?

eddyb (Apr 06 2020 at 21:56, on Zulip):

but still only visits reachable blocks?

centril (Apr 07 2020 at 08:16, on Zulip):

eddyb said:

yeah in codegen

That's what @Santiago Pastorino's PR does atm. I don't understand how that is correct. What if codegen never runs...? Then, AIUI you wouldn't get any error.

centril (Apr 07 2020 at 08:18, on Zulip):

It's important that errors happen in the analysis "phase" of the compiler. Whether or not MIR optimizations are run or codegen is run shouldn't affect this

oli (Apr 07 2020 at 09:18, on Zulip):

we still need to run it during codegen because some error are only triggered after monomorphization since TooGeneric errors are silent

oli (Apr 07 2020 at 09:18, on Zulip):

so we need both

eddyb (Apr 07 2020 at 09:45, on Zulip):

@centril this change doesn't move anything, it just makes it easier to be correct in the future wrt not losing track of consts

centril (Apr 07 2020 at 09:46, on Zulip):

@eddyb sure

centril (Apr 07 2020 at 09:47, on Zulip):

@eddyb Me and @oli were discussing privately re. doing it in run_optimization_passes vs. having some RPO traversal in analysis

centril (Apr 07 2020 at 09:47, on Zulip):

I feel that doing it in run_optimization_passes is fragile as it relies on e.g., SimplifyCfg and other passes not declaring too much code dead and whatnot

centril (Apr 07 2020 at 09:47, on Zulip):

Fragile as in "stability hazard"

eddyb (Apr 07 2020 at 09:49, on Zulip):

you don't need RPO, RPO is for a specific order which preserves dominance (very important to SSA-like stuff)

eddyb (Apr 07 2020 at 09:49, on Zulip):

eddyb said:

actually do we have a cheaper VecDeque+BitSet traversal that doesn't preserve dominator ordering?

^^ see this

eddyb (Apr 07 2020 at 09:49, on Zulip):

you just need "visit all reachable blocks"

eddyb (Apr 07 2020 at 09:50, on Zulip):

and yeah I favor something that can just run immediately after NLL and doesn't depend on whatever SimplifyCfg might do

eddyb (Apr 07 2020 at 09:50, on Zulip):

like if false should not be considered dead code. although. hmm

eddyb (Apr 07 2020 at 09:50, on Zulip):

why do we want to ignore constants in dead code anyway?

oli (Apr 07 2020 at 09:54, on Zulip):

idk, lang team decided we want to

eddyb (Apr 07 2020 at 09:57, on Zulip):

do we remove this dead code before NLL? does NLL just not walk it?

eddyb (Apr 07 2020 at 09:57, on Zulip):

because we could do the gathering even earlier I suspect

centril (Apr 07 2020 at 09:59, on Zulip):

@eddyb consider e.g.:

return;
let x = 0;
let y = &'static _ = &x; // OK because its dead code
centril (Apr 07 2020 at 09:59, on Zulip):

@eddyb Right; I basically also mean "visit all reachable blocks and collect their constants"

centril (Apr 07 2020 at 10:00, on Zulip):

running immediately after NLL seems sensible

eddyb (Apr 07 2020 at 10:00, on Zulip):

anyway yeah I agree with not relying on SimplifyCfg

oli (Apr 07 2020 at 10:02, on Zulip):

so... if we move the mir opt pass doing the collection all the way to the front of the optimization list and make it look for reachable constants manually, would that be better? or should we move this into the mir_validated query?

eddyb (Apr 07 2020 at 10:02, on Zulip):

I'd say mir_validated

oli (Apr 07 2020 at 10:03, on Zulip):

ok

eddyb (Apr 07 2020 at 10:03, on Zulip):

just because it pertains to that aspect

Last update: Sep 28 2020 at 15:45UTC