Stream: t-compiler

Topic: New dataflow framework review (#65672)


ecstatic-morse (Jan 16 2020 at 22:15, on Zulip):

Hey @pnkfelix. If you wanted to chat about #65672, specifically why GenKill is implemented for BitSet, I'd be up for a quick sync discussion sometime in the next few days. Migrating the borrow-check analyses resulted in a ~0.5% reduction in instruction count for clean builds of many non-trivial benchmarks, so this has moved up my list of priorities somewhat. Assuming that I haven't broken anything, my guess is that the reductions stem from overhead around FlowsAtLocation, which is no longer used.

pnkfelix (Jan 17 2020 at 01:09, on Zulip):

Can you ... just write a comment somewhere about why GenKill (and how) is implemented for BitSet?

pnkfelix (Jan 17 2020 at 01:10, on Zulip):

My memory is that I couldn't find any uses of that implementation anywhere, and so I was figuring rather than puzzle over its meaning/purpose, instead we can just get rid of it

pnkfelix (Jan 17 2020 at 01:12, on Zulip):

or wait, maybe I confused myself and was thinking that trait GenKill is the transfer function, rather than an abstraction over the state vector

pnkfelix (Jan 17 2020 at 01:13, on Zulip):

let me double-check the PR again. If trait GenKill is just meant to abstract over the choice of how to represent the state vector, then all we have is a small documentation issue, not a design problem.

pnkfelix (Jan 17 2020 at 01:14, on Zulip):

or wait, maybe I confused myself and was thinking that trait GenKill is the transfer function, rather than an abstraction over the state vector

(but then again, I can't blame myself for thinking this, since you did impl GenKill for GenKillSet, where the latter does indeed represent the transfer function.)

pnkfelix (Jan 17 2020 at 01:15, on Zulip):

I still think you should kill off trait GenKill, following a principle of YAGNI. Or in my own parlance: wait until you actually have one (and preferably more than one) implementers of a trait before you introduce it.

pnkfelix (Jan 17 2020 at 01:17, on Zulip):

(and yes, I know you do have two implementors here. But I don't think you need them.)

pnkfelix (Jan 17 2020 at 01:23, on Zulip):

I'm reading over the PR a bit more carefully now, in any case, to try to reconstruct the reasoning behind the abstraction

pnkfelix (Jan 17 2020 at 01:24, on Zulip):

The GenKill abstraction is only used in a parametric manner in the methods of GenKillAnalysis, here.

pnkfelix (Jan 17 2020 at 01:27, on Zulip):

and I think the only place where you instantiate it as a GenKillSet (rather than as a BitSet) is here, in new_gen_kill. Is that true, or am I missing something?

pnkfelix (Jan 17 2020 at 01:29, on Zulip):

So you are using the trait GenKill for abstract over both the representation of an accumulated transfer function and over the represent the state vector). I'm still musing about the right way to document this.

pnkfelix (Jan 17 2020 at 01:31, on Zulip):

(because I can at least comprehend the motivation, of 1. wanting to ensure that people only write the definition of their transfer functions once, but also 2. not wanting to have to construct a GenKillSet representing the transfer function, only to then immediately apply it a single time to a BitSet before discarding it. Better to just operate on the BitSet directly in that case.

pnkfelix (Jan 17 2020 at 01:37, on Zulip):

So I'm just thinking that the documentation needs to be tweaked to make this design more immediately clear, both in terms of motivation and also in terms of what the relevant abstractions are.

ecstatic-morse (Jan 17 2020 at 05:59, on Zulip):

and I think the only place where you instantiate it as a GenKillSet (rather than as a BitSet) is here, in new_gen_kill. Is that true, or am I missing something?

Yes, this is true. It will be used in more places in the future, but I can worry about that later. This is the place where we compute the cumulative block transfer function and save it for later.

ecstatic-morse (Jan 17 2020 at 06:00, on Zulip):

So you are using the trait GenKill for abstract over both the representation of an accumulated transfer function and over the represent the state vector). I'm still musing about the right way to document this.

Correct. This is the core idea.

ecstatic-morse (Jan 17 2020 at 06:07, on Zulip):

So I'm just thinking that the documentation needs to be tweaked to make this design more immediately clear, both in terms of motivation and also in terms of what the relevant abstractions are.

Would listing the two use-cases help? Sometimes you want to build up a cumulative transfer function and sometimes you don't care about about the transfer function itself and just need the dataflow state at a certain point.

ecstatic-morse (Jan 17 2020 at 06:07, on Zulip):

Basically your second-to-last message above.

ecstatic-morse (Jan 20 2020 at 18:47, on Zulip):

@pnkfelix I added some docs to #65672 to address your concerns. Also, the latest perf numbers for the new framework are good, with an ~1.0% reduction in instruction count during check builds for many real-world crates.

Last update: Jun 07 2020 at 09:30UTC