[Mesa-dev] [PATCH 6/7] nir: add shader reference counting

Thu Nov 5 14:58:29 PST 2015

On Thu, Nov 5, 2015 at 10:53 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Thu, Nov 5, 2015 at 1:13 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Sat, Oct 24, 2015 at 10:07 AM, Rob Clark <robdclark at gmail.com> wrote:
>>> From: Rob Clark <robclark at freedesktop.org>
>>>
>>> For gallium, at least, we'll need this to manage shader's lifetimes,
>>> since in some cases both the driver and the state tracker will need
>>> to hold on to a reference for variant managing.
>>>
>>> Use nir_shader_mutable() before doing any IR opt/lowering/etc, to
>>> ensure you are not modifying a copy someone else is also holding a
>>> reference to.  In this way, unnecessary nir_shader_clone()s are
>>> avoided whenever possible.
>>>
>>> TODO: Any places missed to s/ralloc_free()/nir_shader_unref()/ outside
>>> of freedreno/ir3?
>>
>> I talked with Ken about this a few days ago, and it doesn't seem like
>> a good idea.  Adding reference counting to NIR means mixing two very
>> different memory management models.  NIR already uses the ralloc model
>> where a shader is tied to a context.  Allowing both ralloc and
>> refcount models is a bad idea because freeing a context could cause
>> the shader to go out-of-scope early from the refcount perspective and
>> refcounting means you don't know what thread the shader is deleted in
>> which is really bad for ralloc.  The only way we could do this is if
>> we only used ralloc internally to NIR and disallowed parenting a
>> nir_shader to anything.  I don't really think we want this, at least
>> not for the i965 driver.
>>
>> The best option here would probably be to make a little flyweight
>> wrapper object for TGSI to use that acts as the ralloc context for a
>> NIR shader and is, itself, reference-counted.  You still have to
>> provide similar guarantees (said object can never have a parent), but
>> it would keep those guarantees obvious.
>
> Hmm, maybe a better idea still (and simpler) would be to just disallow
> nir_shader_ref() if parent memctx is not null?

Yeah, we could do that.  However, we have no way of doing a similar
check to say you can't reparent it if refcount > 1.

> This way we can keep the centralized checks for refcnt>1 in the
> nir-pass runner function/macro/whatever..

Meh.  Call the "get me a unique shader" function in your backend
before you do any transformations on it..  There's no reason to call
it multiple times.  If the refcount goes from 1 to > 1 during your
optimization loop, you're sunk anyway.
--Jason

> BR,
> -R
>
>> Sorry :-/
>> --Jason
>>
>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>> ---
>>>  src/gallium/drivers/vc4/vc4_program.c |  2 +-
>>>  src/glsl/nir/nir.c                    |  2 ++
>>>  src/glsl/nir/nir.h                    | 43 +++++++++++++++++++++++++++++++++++
>>>  src/mesa/program/program.c            |  3 ++-
>>>  4 files changed, 48 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/vc4/vc4_program.c b/src/gallium/drivers/vc4/vc4_program.c
>>> index 5d7564b..9a055df 100644
>>> --- a/src/gallium/drivers/vc4/vc4_program.c
>>> +++ b/src/gallium/drivers/vc4/vc4_program.c
>>> @@ -1723,7 +1723,7 @@ vc4_shader_ntq(struct vc4_context *vc4, enum qstage stage,
>>>                          c->num_uniforms);
>>>          }
>>>
>>> -        ralloc_free(c->s);
>>> +        nir_shader_unref(c->s);
>>>
>>>          return c;
>>>  }
>>> diff --git a/src/glsl/nir/nir.c b/src/glsl/nir/nir.c
>>> index 2defa36..53a11f5 100644
>>> --- a/src/glsl/nir/nir.c
>>> +++ b/src/glsl/nir/nir.c
>>> @@ -36,6 +36,8 @@ nir_shader_create(void *mem_ctx,
>>>  {
>>>     nir_shader *shader = ralloc(mem_ctx, nir_shader);
>>>
>>> +   p_atomic_set(&shader->refcount, 1);
>>> +
>>>     exec_list_make_empty(&shader->uniforms);
>>>     exec_list_make_empty(&shader->inputs);
>>>     exec_list_make_empty(&shader->outputs);
>>> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
>>> index 926747c..3ab720b 100644
>>> --- a/src/glsl/nir/nir.h
>>> +++ b/src/glsl/nir/nir.h
>>> @@ -34,6 +34,7 @@
>>>  #include "util/ralloc.h"
>>>  #include "util/set.h"
>>>  #include "util/bitset.h"
>>> +#include "util/u_atomic.h"
>>>  #include "nir_types.h"
>>>  #include "shader_enums.h"
>>>  #include <stdio.h>
>>> @@ -1546,6 +1547,8 @@ typedef struct nir_shader_info {
>>>  } nir_shader_info;
>>>
>>>  typedef struct nir_shader {
>>> +   int refcount;
>>> +
>>>     /** list of uniforms (nir_variable) */
>>>     struct exec_list uniforms;
>>>
>>> @@ -1894,6 +1897,46 @@ void nir_print_instr(const nir_instr *instr, FILE *fp);
>>>
>>>  nir_shader * nir_shader_clone(void *mem_ctx, const nir_shader *s);
>>>
>>> +static inline void
>>> +nir_shader_ref(nir_shader *shader)
>>> +{
>>> +   p_atomic_inc(&shader->refcount);
>>> +}
>>> +
>>> +static inline void
>>> +nir_shader_unref(nir_shader *shader)
>>> +{
>>> +   if (p_atomic_dec_zero(&shader->refcount)) {
>>> +      ralloc_free(shader);
>>> +   }
>>> +}
>>> +
>>> +/* A shader with only a single reference is mutable: */
>>> +static inline bool
>>> +nir_shader_is_mutable(nir_shader *shader)
>>> +{
>>> +   return p_atomic_read(&shader->refcount) == 1;
>>> +}
>>> +
>>> +/* Convert a shader reference into a mutable shader reference.  Ie. if
>>> + * there is only a single reference to the shader, then return that,
>>> + * otherwise clone and drop reference to existing shader.
>>> + *
>>> + * TODO maybe an assert that shader->refcnt == 1 in the various opt/
>>> + * lowering passes?  If only there was a good central place to put it.
>>> + */
>>> +static inline nir_shader *
>>> +nir_shader_mutable(nir_shader *shader)
>>> +{
>>> +   if (nir_shader_is_mutable(shader)) {
>>> +      return shader;
>>> +   } else {
>>> +      nir_shader *ns = nir_shader_clone(ralloc_parent(shader), shader);
>>> +      nir_shader_unref(shader);
>>> +      return ns;
>>> +   }
>>> +}
>>> +
>>>  #ifdef DEBUG
>>>  void nir_validate_shader(nir_shader *shader);
>>>  #else
>>> diff --git a/src/mesa/program/program.c b/src/mesa/program/program.c
>>> index 0e78e6a..c2da66e 100644
>>> --- a/src/mesa/program/program.c
>>> +++ b/src/mesa/program/program.c
>>> @@ -38,6 +38,7 @@
>>>  #include "prog_parameter.h"
>>>  #include "prog_instruction.h"
>>>  #include "util/ralloc.h"
>>> +#include "nir.h"
>>>
>>>
>>>  /**
>>> @@ -273,7 +274,7 @@ _mesa_delete_program(struct gl_context *ctx, struct gl_program *prog)
>>>     }
>>>
>>>     if (prog->nir) {
>>> -      ralloc_free(prog->nir);
>>> +      nir_shader_unref(prog->nir);
>>>     }
>>>
>>>     mtx_destroy(&prog->Mutex);
>>> --
>>> 2.5.0
>>>