[Mesa-dev] [PATCH 02/11] nir: Add a pass-running infastructure
Connor Abbott
cwabbott0 at gmail.com
Wed Oct 28 18:15:10 PDT 2015
On Wed, Oct 28, 2015 at 8:04 PM, Ian Romanick <idr at freedesktop.org> wrote:
> 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.
I didn't "unilaterally decide to change it" at all. It was the style I
was more familiar with, and as I explained, it saved me time and I
liked it better, so I did it. I didn't even have experience with Mesa
then outside GLSL IR, which obviously doesn't have this concern, so I
didn't consider whether it meshed with the rest of Mesa. There were
several points before the code was merged where someone could've
complained loud enough to get it changed, but no one did. Most
significantly, no one else significantly involved with core NIR has
felt strongly enough to complain about it since. But now that NIR is a
fairly large project with more than 30kloc (according to sloccount),
do you really want to mass-rename everything? Whatever the extra time
needed to acclimatize yourself to NIR vs. time/frustration typing and
line-wrapping, the time spent bikeshedding on this and the pain of
rewriting everything and muddling up the history is going to be much
larger. But if you're only here to complain, then we're wasting time
here too.
>
> 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