[Mesa-dev] [PATCH 02/11] nir: Add a pass-running infastructure
Ian Romanick
idr at freedesktop.org
Wed Oct 28 17:04:07 PDT 2015
On 10/28/2015 04:43 PM, Connor Abbott 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
> 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
But that it is the way *this* project has done things. We made a
conscious decision to do them that way. I don't think you guys had any
right to unilaterally decide to change that.
Saying that not having to type 'struct' is some huge time savings is
just pure rubbish. I have a lot of trouble believing that you could
even say it out loud with a straight face. Seriously.
What is real is the difficulty of maintaining a project with 238
different coding styles. As soon as look in a different part of the
code, you have to relearn things in order to get any work done. This is
already a really, really big problem in Mesa. We constantly give people
review feedback like, "You did this the way other things in that file
are done, but that's not how we do it in the rest of the project now.
Please change."
I'll go out on a limb and guess that the amount of time spent trying to
figure out what the coding style is in an particular part of the project
is, rejecting patches that didn't properly meet those standards in spite
of the author's best effort, and re-spinning patches outweighs the time
spent typing 'struct' by a margin of 10:1.
People often complain about the way that open-source projects are so nit
picky about coding standards. It's a necessary survival tactic. Mesa
is 22 years old. It has had dozens of contributors. Imagine how much
worse it would be if every contributor, or even just the major
contributors, got to change aspects of the coding standards that they
didn't like or thought was too much work. It would be a completely
maintainable disaster. I plan to work on this project for another 14
years, and I'm going to do my best to keep it from falling apart into
that disaster.
> 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
>
More information about the mesa-dev
mailing list