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

[Jason Ekstrand]

On Dec 19, 2015 5:55 PM, "Rob Clark" <robdclark at gmail.com> wrote:
>
> From: Rob Clark <robclark at freedesktop.org>
>
> Jason,
>
> How much do you hate this idea?  Seems like an easy alternative to
> using ralloc ctx's to clean up nir variants/clones, which would let
> us drop the parent memctx for nir_shader_create()/clone(), making
> it easier to introduce reference counting.

I think "hate" is a but strong. I don't like it but it works.  If we really
want nir_shader refcounted, we'll have to do something.

About refcounting... The more I think about it the more I'm not convinced
it's useful.  As it stands, we have no use for it an I'm not convinced you
do either.  We'll see if I can convince you. :-)

In the history of i965 using NIR, we've had about three different ways of
doing things:

1) GLSL is the gold copy and we run glsl_to_nir for every shader/variant
compile.  This is what we did when we first stated using NIR because it was
easy and didn't involve reworking any plumbing.

2) Lowered NIR is the gold copy; variants are done entirely in the back-end
IR.  This is what we did up until about a month ago.  Because variants are
done in the back-end, we can run gksl_to_nir and do all of our optimizing
and lowering at link time.  Going from NIR to the final shader binary is
then a read-only operation as far as NIR is concerned.

3) Optimized but not lowered NIR is the gold copy; variants are sometimes
done in NIR.  This is the scheme we use now.  We call glsl_to_nir and do
some of the optimization and lowering at link time but leave it in SSA
form.  When we go to compile the final shader, we make a copy, apply
variants, do the final lowering and then go into the back-end IR.

In each of these cases, we know exactly where we need to make a copy
without the help of reference counting.  In the first, we get a fresh copy
each time so we are free to destroy the copy.  In the second, we never have
to modify the NIR so no copy.  In the third scheme, we always have to make
a copy because, even if variants are a no-op, we still have to go out of
SSA form and do final lowering.  You could say that we could avoid making
that copy.  However, the work to determine when we don't need variants and
can do all our lowering up-front is far more than the work saved by
reference counting.

How about gallium?  Here's how I imagine it would work (please correct me
of I'm wrong):

1) In the TGSI case, tgsi_to_nir gets called for each compile so you get a
fresh mutable shader each time.  In this case, you are free to do whatever
you want with the shader without making a copy.

2) In the GLSL case, you run glsl_to_nir and do some basic optimizations at
link time and hold onto the NIR shader. (Hold a reference of you'd like.)
When you go to compile it in the back-end, it needs to do it's own lowering
so it takes a reference and ends up making a copy.

If this description is anywhere close to correct, then I don't think you
really need it either.  Determining whether or not you need to copy is
simply "if (comes_from_tgsi)”.  Maybe there's something subtle about the
gallium layer that I don't know that makes refcounting the best solution.
Please enlighten me of there is.
--Jason

> ---
>  src/mesa/drivers/dri/i965/brw_compiler.h          | 22
++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/brw_fs.cpp              |  6 ++++--
>  src/mesa/drivers/dri/i965/brw_vec4.cpp            |  3 ++-
>  src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp |  3 ++-
>  4 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_compiler.h
b/src/mesa/drivers/dri/i965/brw_compiler.h
> index c9e0317..eb1087a 100644
> --- a/src/mesa/drivers/dri/i965/brw_compiler.h
> +++ b/src/mesa/drivers/dri/i965/brw_compiler.h
> @@ -26,6 +26,7 @@
>  #include <stdio.h>
>  #include "brw_device_info.h"
>  #include "main/mtypes.h"
> +#include "nir.h"
>
>  #ifdef __cplusplus
>  extern "C" {
> @@ -651,6 +652,27 @@ struct brw_gs_prog_data
>
>  /** @} */
>
> +#ifdef __cplusplus
> +/* A common pattern is for brw_compile_xyz to create a clone of the
original
> + * shader, modify it (various lowering passes, etc), and then throw it
away.
> + * The scoped_clone helper class is just a way to use RAII to clean up
the
> + * shader when it goes out of scope.
> + */
> +class scoped_clone
> +{
> +public:
> +   nir_shader *shader;
> +   scoped_clone(const nir_shader *s)
> +   {
> +      shader = nir_shader_clone(NULL, s);
> +   }
> +   ~scoped_clone()
> +   {
> +      ralloc_free(shader);
> +   }
> +};
> +#endif
> +
>  /**
>   * Compile a vertex shader.
>   *
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index c833ef0..44ea156 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -5490,7 +5490,8 @@ brw_compile_fs(const struct brw_compiler *compiler,
void *log_data,
>                 unsigned *final_assembly_size,
>                 char **error_str)
>  {
> -   nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> +   scoped_clone sc(src_shader);
> +   nir_shader *shader = sc.shader;
>     shader = brw_nir_apply_sampler_key(shader, compiler->devinfo,
&key->tex,
>                                        true);
>     shader = brw_postprocess_nir(shader, compiler->devinfo, true);
> @@ -5616,7 +5617,8 @@ brw_compile_cs(const struct brw_compiler *compiler,
void *log_data,
>                 unsigned *final_assembly_size,
>                 char **error_str)
>  {
> -   nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> +   scoped_clone sc(src_shader);
> +   nir_shader *shader = sc.shader;
>     shader = brw_nir_apply_sampler_key(shader, compiler->devinfo,
&key->tex,
>                                        true);
>     shader = brw_postprocess_nir(shader, compiler->devinfo, true);
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp
b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> index a697bdf..a89e884 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp
> @@ -1957,7 +1957,8 @@ brw_compile_vs(const struct brw_compiler *compiler,
void *log_data,
>                 unsigned *final_assembly_size,
>                 char **error_str)
>  {
> -   nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> +   scoped_clone sc(src_shader);
> +   nir_shader *shader = sc.shader;
>     shader = brw_nir_apply_sampler_key(shader, compiler->devinfo,
&key->tex,
>
 compiler->scalar_stage[MESA_SHADER_VERTEX]);
>     shader = brw_postprocess_nir(shader, compiler->devinfo,
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> index b13d36e..9ed7e0d 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_gs_visitor.cpp
> @@ -617,7 +617,8 @@ brw_compile_gs(const struct brw_compiler *compiler,
void *log_data,
>     memset(&c, 0, sizeof(c));
>     c.key = *key;
>
> -   nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> +   scoped_clone sc(src_shader);
> +   nir_shader *shader = sc.shader;
>     shader = brw_nir_apply_sampler_key(shader, compiler->devinfo,
&key->tex,
>
 compiler->scalar_stage[MESA_SHADER_GEOMETRY]);
>     shader = brw_postprocess_nir(shader, compiler->devinfo,
> --
> 2.5.0
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151220/4ac9853b/attachment.html>