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.
> 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
