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

[Jason Ekstrand]

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.

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
>