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

[Connor Abbott]

On Wed, Oct 28, 2015 at 8:00 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Wed, Oct 28, 2015 at 4:26 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>> While it works for now, I'm not so sure about how we're conflating the
>> definition of a pass and its dynamic instantiation. Eventually we'll
>> want to have passes refer to each other ("please run pass B after pass
>> A to clean up its mess", etc.), and in that case we need to be able to
>> have to the pass itself add a pointer to another pass inside its pass
>> struct, and then look up the dynamic pass associated with it after
>> we're done loading all the passes (if it exists). That would be easier
>> if we removed the closure pointer from nir_pass and made it a purely
>> static thing, and instead added a new struct or made the closure
>> argument be passed around separately. Otherwise we'd have to match the
>> contents of the struct, which doesn't seem like the right thing to do.
>
> Yeah, I thought about that.  Unfortunately, I haven't come up with a
> good way to make it work easily.  One option would be to have another
> struct with a pointer to the pass and the data but that makes building
> a pass list kind of messy.  I also considered not having a closure at
> all and just putting everything in nir_compiler_options but that's not
> a good idea either as it will pollute the nir_compiler_options struct
> and doesn't allow for custom passes with options.
>
> Here's another option which I hadn't previously considered:  Pass in a
> list of pass-data pairs something like
>
> struct {
>    nir_pass *pass;
>    void *data;
> }
>
> where you only have to pass data in for the passes that need it.  That
> way the list of passes and the extra arguments provided are completely
> orthogonal.  It would mean either a hash lookup or a linear walk over
> the pass data for every pass, but you should only have 3 or 4 things
> in there and we can add a field to nir_pass to indicate whether or not
> a pass takes arguments.  The over-all overhead should be minimal.  It
> does require, however, that the nir_pass pointers are unique which
> shouldn't be a problem.

Well, the other option to consider would be to not pass in an array at
all and instead have functions to construct a pass manager and then
have a function to add a pass (with associated void  * pointer) to it
like in LLVM. You could even do C-style subclassing instead of passing
a pointer, if you wanted to. The advantage here is that adding passes
is more flexible, since it's not in one giant array: for example, you
could have a function like nir_add_common_passes() that adds DCE, copy
prop, CSE, and other stuff that every driver should have, and then
have the drivers call it instead of duplicating them 3 times (and
having to add every pass 3 times, and sometimes forgetting to!).

As to resolving static pass instances to dynamic ones: I think this
can be done at pass creation time, or at least before the loop runs.
You keep an internal array of pass datastructures that are augmented
with references to the instantion of the pass that you got handed, if
it exists, and initialize it after you've added all the passes but
before running them. Of course, we don't have to do this until later
though.

>
>> On Wed, Oct 28, 2015 at 5:32 PM, Jason Ekstrand <jason at jlekstrand.net> 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 {
>>> +   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;
>>> +}
>>> --
>>> 2.5.0.400.gff86faf
>>>
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev