On Wed, Oct 28, 2015 at 8:51 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> On Wed, Oct 28, 2015 at 5:41 PM, Connor Abbott <cwabbo...@gmail.com> wrote:
>> On Wed, Oct 28, 2015 at 8:00 PM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
>>> On Wed, Oct 28, 2015 at 4:26 PM, Connor Abbott <cwabbo...@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.
Ok, fair enough. I think adding a separate struct with the data
pointer like you said sounds like the way to go for now, although I
can see how it could be somewhat painful until/unless we have a
separate pass builder object.
>
>>>
>>>> On Wed, Oct 28, 2015 at 5:32 PM, Jason Ekstrand <ja...@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@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
