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

[Rob Clark]

On Sat, Dec 19, 2015 at 9:01 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> Why don't you just introduce a reference-counted wrapper around a
> nir_shader * and pass that around instead? This seems like a
> gallium-specific problem, so modifying other things to work around it
> doesn't seem so appealing.

1) I wanted to keep the _is_mutable() asserts in nir
2) Jason mentioned at one point on #dri-devel that it would to have
_is_mutable() to know if he could fast-pass and skip the clone..

since the parent memctx is only pretty trivially used right now, and
can be reasonably trivially removed (if not this way, by instead just
pulling the nir_shader_clone() outside of brw_compile_xyz() and
passing in the clone instead of the original) refcnt'ing seems like
the more useful thing to have

BR,
-R

> On Sat, Dec 19, 2015 at 8: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.
>>
>> ---
>>  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
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev