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

[Kristian Høgsberg]

On Wed, Oct 28, 2015 at 4:43 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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

I'm not arguing we should change anything at this point, but I did
object at the time nir was in review and pointed out that you were
breaking with mesa style. Most typedef uses involve some kind of
convention for how to indicate that a "naked" identifier is actually a
type - often using camel case or appending _t or _type or such (even
appending _struct in some cases!). My point is that C already gives
you a perfectly fine mechanism for this: the struct keyword. Instead
of coming up with some half-baked convention, use what C gives you.

> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev