[Mesa-dev] [RFC] i965: alternative to memctx for cleaning up nir variants

[Rob Clark]

On Mon, Dec 28, 2015 at 3:51 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> On Mon, Dec 28, 2015 at 3:31 PM, Rob Clark <robdclark at gmail.com> wrote:
>> On Mon, Dec 28, 2015 at 1:58 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>> On Mon, Dec 28, 2015 at 1:35 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>> On Mon, Dec 28, 2015 at 12:37 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>>>> On Mon, Dec 28, 2015 at 10:13 AM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>> On Tue, Dec 22, 2015 at 10:11 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>>>>>> On Tue, Dec 22, 2015 at 9:55 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>>>> On Tue, Dec 22, 2015 at 9:47 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>>>>>>>> On Tue, Dec 22, 2015 at 9:02 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>>>>>> On Mon, Dec 21, 2015 at 1:48 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>>>>>>>>>>
>>>>>>>>>>> I think two different concepts of ownership are getting conflated here:
>>>>>>>>>>> Right/responsibility to delete and right to modify.
>>>>>>>>>>>
>>>>>>>>>>> The way I understand it, gallium, as it stands, gives neither to the driver.
>>>>>>>>>>> A back-end using NIR requires the right to modify but who deletes it doesn't
>>>>>>>>>>> ultimately matter.  I think it's dangerous to pass one of these rights to
>>>>>>>>>>> the driver and not the other but we need to think about both.
>>>>>>>>>>
>>>>>>>>>> yeah, uneasy about driver modifying the IR if the state tracker is
>>>>>>>>>> still going to potentially spin off variants of the IR.. that sounds
>>>>>>>>>> like madness.
>>>>>>>>>>
>>>>>>>>>> The refcnt'ing I proposed does deal w/ right to modify vs delete via
>>>>>>>>>> nir_shader(_is)_mutable() which returns something that is guaranteed
>>>>>>>>>> to be safe to modify (ie. has only a single reference)
>>>>>>>>>>
>>>>>>>>>>> What I'm trying to say is that we have two options here:
>>>>>>>>>>>
>>>>>>>>>>> 1) gallium passes IR to the back-end that it is free to modify and is
>>>>>>>>>>> required to delete when it's done.
>>>>>>>>>>
>>>>>>>>>> with refcnt'ing, s/delete/unref/
>>>>>>>>>>
>>>>>>>>>> The idea is, the st transfers ownership of the reference it passes to
>>>>>>>>>> the driver.  If the st wants to hang on to a reference itself, it must
>>>>>>>>>> increment the refcnt before passing to the driver (backend).
>>>>>>>>>>
>>>>>>>>>> Without refcnt'ing, I suppose we could (since we don't have to follow
>>>>>>>>>> TGSI semantics), just decree that the driver always takes ownership of
>>>>>>>>>> the copy passed in, and if the st wants to hang on to a copy too, then
>>>>>>>>>> it must clone.  I suppose this would work well enough for
>>>>>>>>>> freedreno/vc4, which both end up generating variants later.  It does
>>>>>>>>>> force an extra clone for drivers that immediately translate into their
>>>>>>>>>> own backend IR and don't need to keep the NIR around, for example.
>>>>>>>>>> Maybe that is not worth caring about (since at this point it is
>>>>>>>>>> hypothetical).
>>>>>>>>>
>>>>>>>>> While always cloning does have this disadvantage, I don't think it's
>>>>>>>>> really relevant here. Even if the driver throws away the NIR
>>>>>>>>> immediately after consuming it, almost invariably it's going to want
>>>>>>>>> to modify  it. The generic NIR passed in by the state tracker (other
>>>>>>>>> IR -> NIR + some optimizations) is almost never going to be the same
>>>>>>>>> as the NIR after going through driver-specific lowering passes, which
>>>>>>>>> means that drivers are never going to want a read-only version of the
>>>>>>>>> IR. In light of that, I think making the driver own the IR passed in
>>>>>>>>> seems like the most sensible thing.
>>>>>>>>
>>>>>>>> well, unless the driver is already doing it's own lowering in it's own
>>>>>>>> native IR..
>>>>>>>
>>>>>>> Well, if you're not doing any lowering in NIR, then you aren't really
>>>>>>> taking any advantage of it. I can't see a plausible scenario where all
>>>>>>> the lowering is done in the driver's own IR -- and as soon as you do
>>>>>>> anything in NIR, you need the driver-owns-IR semantics.
>>>>>>
>>>>>> When it comes to shader variants, I have a mix, with some things
>>>>>> lowered in nir and others just handled in backend..
>>>>>>
>>>>>> The re-work / cleanup that I have had on a branch for a while now
>>>>>> (since it is currently blocked on refcnt'ing) does a first round of
>>>>>> variant-key independent NIR lowering/opt passes.  And then at draw
>>>>>> time, if the variant key has anything that is lowered in nir, I do a
>>>>>> second round.
>>>>>>
>>>>>>>>
>>>>>>>> Maybe it is too much of a hypothetical.. I still think refcnt'ing
>>>>>>>> gives some nice flexibility to deal with various scenarios, and having
>>>>>>>> to call nir_shader_unref() isn't so much of a burden.
>>>>>>>
>>>>>>> Still, I can't see how this flexibility is at all useful, and it seems
>>>>>>> like overkill since the driver will always want a mutable version of
>>>>>>> the IR anyways.
>>>>>>
>>>>>> Well, due to the structure I mentioned above, at draw time when I need
>>>>>> to generate a variant with nothing lowered in NIR, I simply incr the
>>>>>> refcnt on the IR which has already gone through first round of NIR
>>>>>> passes, and pass that in to my back end compiler.  At the end, once
>>>>>> the shader binary is generated, I can unconditionally unref the
>>>>>> nir_shader without having to care.
>>>>>>
>>>>>> Without refcnt'ing I'd either have to generate a pointless clone or
>>>>>> keep track that the nir_shader should not actually be free'd.  Not
>>>>>> impossible, just a bit more ugly.
>>>>>
>>>>> Assuming you do all your variant management in your driver's IR, then
>>>>> you don't need to do anything. If you do some variant management in
>>>>> NIR, then in the function where you do the NIR-based lowering you can
>>>>> check if you need to do any lowering based on the shader key, clone
>>>>> first, and give the NIR->ir3 function the cloned IR and then free it.
>>>>> It might be a "bit more ugly," but it's really not that much different
>>>>> from the refcounting, and when the extra shader gets created/freed is
>>>>> made explicit.
>>>>
>>>> I'd have to re-arrange things some compared to how the backend works
>>>> now..  not impossible, but annoying.  Also, I could more easily unref
>>>> the NIR once I've converted into ir3 rather than waiting until after
>>>> the backend passes.
>>>
>>> That's just a matter of how you structure things... if you do the
>>> NIR-based lowering in the same function as converting to ir3, which
>>> seems like a good idea anyways, then it should be possible to unref
>>> before you run your backend passes. But I doubt the difference matters
>>> anyways. Our experience with i965 is that holding a lot of shaders at
>>> once can eat into memory, but memory consumption while actually doing
>>> work hardly matters (I guess it could if it's totally out of control,
>>> but that's not here...).
>>
>> well, at a minimum, not having refcnt'ing is constraining the way the
>> code is structured..
>>
>>>>
>>>>>>
>>>>>> (The gallium glsl_to_nir stuff is also currently using refcnt'ing,
>>>>>> although at least for freedreno/ir3 it isn't strictly needed.. I could
>>>>>> just unconditionally clone in the state tracker.  That said, I'm still
>>>>>> of the opinion that refcnt'ing could be useful to some other driver
>>>>>> someday)
>>>>>
>>>>> "It could be useful to some driver someday" isn't a good argument for
>>>>> adding stuff today. We've already had enough examples of things in NIR
>>>>> that we added because we thought it was useful, but turned out not to
>>>>> be.
>>>>
>>>> well, it is useful to one driver today, as I explained..  so it is
>>>> more a matter of "It could be also useful to some other driver...".
>>>> Otherwise I would agree with you.
>>>
>>> But even for you, this is still overkill. Reference counting is most
>>> useful when there are potentially many different owners of a resource,
>>> which isn't the case here. If we add reference counting now, we'd be
>>> adding an essentially useless feature, and the fact that you used it
>>> doesn't change the fact that it's unnecessary.
>>
>> well, I guess it depends on how you define "owners".. I mean, just
>> because something is one driver, doesn't mean things can't be
>> structured/modularized to separate concerns.  Reference counting gives
>> some nice flexibility to avoid copies for expensive data structures
>> and lets you deal w/ (sub)modules owning references rather than the
>> entire data structure.
>
> I would agree with you, if the language gave us support for dealing
> with references automatically. As it is, you have to manually call
> ref() and unref() in the right places, which isn't much better than
> passing in a "does the callee own the shader" flag (which we usually
> know ahead of time anyways).

well, we deal w/ refcnt'ing all the time in the graphics stack..  I
guess it just doesn't seem like such a difficult/foreign concept to
me.

>>
>> Not having reference counting constrains the way the code is
>> structured, and (still, IMHO) for no particularly good reason.
>
> And adding reference counting forces us to change i965 for no
> particularly good reason. I guess whether you like reference counting
> is mostly a matter of taste, but I'd prefer keeping it simple.

well, I'd ofc handle the minor i965 changes myself.. but I guess we
are to the point of arguing about taste..

BR,
-R

>>
>> BR,
>> -R
>>
>>>>
>>>> BR,
>>>> -R
>>>>
>>>>>>
>>>>>> BR,
>>>>>> -R
>>>>>>
>>>>>>>>
>>>>>>>> BR,
>>>>>>>> -R
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> (I guess nouveau is the one driver, that if it ever consumed NIR,
>>>>>>>>>> would translate immediately into it's own backend IR?)
>>>>>>>>>>
>>>>>>>>>>> 2) gallium passes read-only IR to the back-end and it always makes a copy.
>>>>>>>>>>
>>>>>>>>>> This is how it is w/ TGSI, but I think with NIR we are free to make a
>>>>>>>>>> clean break.  And we *definitely* want to avoid
>>>>>>>>>> the-driver-always-copies semantics..
>>>>>>>>>>
>>>>>>>>>>> It sounds like, from what Marek is saying, that gallium is currently doing
>>>>>>>>>>> (2) and changing it to (1) would be painful.  I think reference counting is
>>>>>>>>>>> more like an awkward option 1.5 than option 3.  Reference counting would
>>>>>>>>>>> mean that gallium passes a reference to the driver which it is expected to
>>>>>>>>>>> unref but may keep a second reference if it wants to keep the driver from
>>>>>>>>>>> modifying it.  Then the driver may or may not make a copy based on the
>>>>>>>>>>> number of references.  Why don't we just make it explicit and add a
>>>>>>>>>>> read-only bit and call it a day.
>>>>>>>>>>>
>>>>>>>>>>> One of the reasons I don't like passing a reference is that it effectively
>>>>>>>>>>> puts allocation and freeing in different components of the driver.
>>>>>>>>>>
>>>>>>>>>> With refcnt'ing you should talk in terms of ref/unref rather than
>>>>>>>>>> allocate/free.. imho.  Although maybe that is what you meant.  (In
>>>>>>>>>> which case, yes, that was my idea, that passing in to driver transfers
>>>>>>>>>> ownership of the passed reference.)
>>>>>>>>>>
>>>>>>>>>>> This
>>>>>>>>>>> means that if and driver doesn't care at all about the shader that gets
>>>>>>>>>>> passed in, it still has to under it to avoid a memory leak.  You can't have
>>>>>>>>>>> the driver take the reference because then, either it comes in with a
>>>>>>>>>>> recount of 0 and should have been deleted, or the "can I modify this" check
>>>>>>>>>>> becomes "recount <= 2" which makes no sense.
>>>>>>>>>>
>>>>>>>>>> hmm, no, if ownership of the reference is transferred to the driver,
>>>>>>>>>> then it becomes "refcount == 1" (and refcount == 0 should be an
>>>>>>>>>> assert, because something has gone horribly wrong)
>>>>>>>>>>
>>>>>>>>>> BR,
>>>>>>>>>> -R