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

[Connor Abbott]

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.

>
> 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.

>
> 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