[Mesa-dev] [PATCH 02/11] nir: Add a pass-running infastructure
Jason Ekstrand
jason at jlekstrand.net
Wed Oct 28 17:51:06 PDT 2015
On Wed, Oct 28, 2015 at 5:41 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
> 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.
Yeah, a pass manager seems a bit heavy-handed for now. The big thing
is not to paint ourselves into a corner. Once we have a pass
infrastructure in place, people can play with it a bit in different
drivers and we can see what we like. At the very least, we should
1) Pull the data pointer out of nir_pass
2) Make nir_shader_optimize take an array of pointers.
I'm not sure what to do about the data pointer. We could pull it out
entirely for the moment but it will have to go back in somewhere. Not
very far into the future, I'm going to want to port the rest of the
passes and then we'll need a real solution.
>>
>>> 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
More information about the mesa-dev
mailing list