[Mesa-dev] [PATCH 02/11] nir: Add a pass-running infastructure

[Connor Abbott]

On Wed, Oct 28, 2015 at 7:06 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 10/28/2015 02:32 PM, Jason Ekstrand wrote:
>> ---
>>  src/glsl/nir/nir.h      | 19 +++++++++++++++
>>  src/glsl/nir/nir_pass.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 82 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/glsl/nir/nir.h b/src/glsl/nir/nir.h
>> index e3777f9..069c7c1 100644
>> --- a/src/glsl/nir/nir.h
>> +++ b/src/glsl/nir/nir.h
>> @@ -1582,6 +1582,25 @@ typedef struct nir_shader {
>>        foreach_list_typed(nir_function_overload, overload, node, \
>>                           &(func)->overload_list)
>>
>> +typedef struct nir_pass {
>
> A couple years ago Kristian, with the support of everyone, waged a war
> on 'typedef struct foo { ... } foo;'  Has this awful idiom really risen
> from the dead?  Can we please send it back to the grave?

(flamewar incoming!)

Yes, it has, and no.

In case you haven't read any NIR code, NIR uses this all over the
place -- all of the core datastructures are typedef'd. The only
argument I've heard against this practice, from Linus, is that it
makes telling if a value is a lightweight thing, like an integer, or
not. But this isn't the kernel; we don't use typedefs for integers at
all, and the only place where we use nir_* and it *isn't* a structure
is the enums. Pretty much the entire time, it's fairly obvious these
are enums, either because they have "type" in the name (e.g.
nir_instr_type), indicating C-style subclassing, or they're some kind
of operation code (nir_op and nir_intrinsic_op). This is fairly
subjective, but if you're not able to look at a nir_* type and know
whether it's a lightweight thing or not, then you probably don't know
the code well enough to understand it anyways. The amount of typing
they save is just way too large compared to the potential
(theoretical) inconvenience of not knowing how the type is declared.
But if this bikeshed seriously disturbs you, then I'd much rather
change the enums to not be typedef'd, since you have to type out their
types a lot less often.

>
>> +   bool (*shader_pass_func)(nir_shader *shader, void *data);
>> +   bool (*impl_pass_func)(nir_function_impl *impl, void *data);
>> +   nir_metadata metadata_preserved;
>> +   void *data;
>> +} nir_pass;
>> +
>> +static inline nir_pass
>> +nir_pass_with_data(nir_pass pass, void *data)
>> +{
>> +   pass.data = data;
>> +   return pass;
>> +}
>> +
>> +bool nir_shader_run_pass(nir_shader *shader, const nir_pass *pass);
>> +bool nir_function_impl_run_pass(nir_function_impl *impl, const nir_pass *pass);
>> +bool nir_shader_optimize(nir_shader *shader,
>> +                         const nir_pass *passes, unsigned num_passes);
>> +
>>  nir_shader *nir_shader_create(void *mem_ctx,
>>                                gl_shader_stage stage,
>>                                const nir_shader_compiler_options *options);
>> diff --git a/src/glsl/nir/nir_pass.c b/src/glsl/nir/nir_pass.c
>> index a03e124..059d016 100644
>> --- a/src/glsl/nir/nir_pass.c
>> +++ b/src/glsl/nir/nir_pass.c
>> @@ -27,7 +27,7 @@
>>  #include "nir.h"
>>
>>  /*
>> - * Handles management of the metadata.
>> + * Handles management of NIR passes and metadata.
>>   */
>>
>>  void
>> @@ -52,3 +52,65 @@ nir_metadata_preserve(nir_function_impl *impl, nir_metadata preserved)
>>  {
>>     impl->valid_metadata &= preserved;
>>  }
>> +
>> +static bool
>> +_function_impl_run_pass(nir_function_impl *impl, const nir_pass *pass)
>> +{
>> +   bool progress = pass->impl_pass_func(impl, pass->data);
>> +   if (progress)
>> +      nir_metadata_preserve(impl, pass->metadata_preserved);
>> +
>> +   return progress;
>> +}
>> +
>> +bool
>> +nir_function_impl_run_pass(nir_function_impl *impl, const nir_pass *pass)
>> +{
>> +   bool progress = _function_impl_run_pass(impl, pass);
>> +
>> +   /* TODO: Add a way to validate a single function_impl */
>> +   nir_validate_shader(impl->overload->function->shader);
>> +
>> +   return progress;
>> +}
>> +
>> +bool
>> +nir_shader_run_pass(nir_shader *shader, const nir_pass *pass)
>> +{
>> +   bool progress;
>> +   if (pass->shader_pass_func) {
>> +      progress = pass->shader_pass_func(shader, pass->data);
>> +
>> +      if (progress) {
>> +         nir_foreach_overload(shader, overload) {
>> +            if (overload->impl)
>> +               nir_metadata_preserve(overload->impl, pass->metadata_preserved);
>> +         }
>> +      }
>> +   } else {
>> +      progress = false;
>> +      nir_foreach_overload(shader, overload) {
>> +         if (overload->impl)
>> +            progress |= _function_impl_run_pass(overload->impl, pass);
>> +      }
>> +   }
>> +
>> +   return progress;
>> +}
>> +
>> +bool
>> +nir_shader_optimize(nir_shader *shader,
>> +                    const nir_pass *passes, unsigned num_passes)
>> +{
>> +   bool progress, total_progress = false;
>> +
>> +   do {
>> +      progress = false;
>> +      for (unsigned p = 0; p < num_passes; p++)
>> +         progress |= nir_shader_run_pass(shader, &passes[p]);
>> +
>> +      total_progress |= progress;
>> +   } while (progress);
>> +
>> +   return total_progress;
>> +}
>>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev