On 10/28/2015 06:15 PM, Connor Abbott wrote:
> On Wed, Oct 28, 2015 at 8:04 PM, Ian Romanick <i...@freedesktop.org> wrote:
>> On 10/28/2015 04:43 PM, Connor Abbott wrote:
>>> On Wed, Oct 28, 2015 at 7:06 PM, Ian Romanick <i...@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
Yes, that's what I said. You made a break from the project norm without
the input or acceptance of other people on the project.
> 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@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
