[Mesa-dev] [PATCH 1/3] nir: Add support for CSE on textures.

[Connor Abbott]

On Wed, Feb 11, 2015 at 3:17 PM, Eric Anholt <eric at anholt.net> wrote:
> Connor Abbott <cwabbott0 at gmail.com> writes:
>
>> On Wed, Feb 11, 2015 at 2:36 PM, Eric Anholt <eric at anholt.net> wrote:
>>> Connor Abbott <cwabbott0 at gmail.com> writes:
>>>
>>>> On Tue, Feb 10, 2015 at 1:32 PM, Eric Anholt <eric at anholt.net> wrote:
>>>>> Connor Abbott <cwabbott0 at gmail.com> writes:
>>>>>
>>>>>> On Sat, Feb 7, 2015 at 12:16 AM, Eric Anholt <eric at anholt.net> wrote:
>>>>>>> NIR instruction count results on i965:
>>>>>>> total instructions in shared programs: 1261954 -> 1261937 (-0.00%)
>>>>>>> instructions in affected programs:     455 -> 438 (-3.74%)
>>>>>>>
>>>>>>> One in yofrankie, two in tropics.  Apparently i965 had also optimized all
>>>>>>> of these out anyway.
>>>>>>> ---
>>>>>>>  src/glsl/nir/nir_opt_cse.c | 43 +++++++++++++++++++++++++++++++++++++++----
>>>>>>>  1 file changed, 39 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/glsl/nir/nir_opt_cse.c b/src/glsl/nir/nir_opt_cse.c
>>>>>>> index b3e9c0d..55dc083 100644
>>>>>>> --- a/src/glsl/nir/nir_opt_cse.c
>>>>>>> +++ b/src/glsl/nir/nir_opt_cse.c
>>>>>>> @@ -80,8 +80,41 @@ nir_instrs_equal(nir_instr *instr1, nir_instr *instr2)
>>>>>>>        }
>>>>>>>        return true;
>>>>>>>     }
>>>>>>> -   case nir_instr_type_tex:
>>>>>>> -      return false;
>>>>>>> +   case nir_instr_type_tex: {
>>>>>>> +      nir_tex_instr *tex1 = nir_instr_as_tex(instr1);
>>>>>>> +      nir_tex_instr *tex2 = nir_instr_as_tex(instr2);
>>>>>>> +
>>>>>>> +      if (tex1->op != tex2->op)
>>>>>>> +         return false;
>>>>>>> +
>>>>>>> +      if (tex1->num_srcs != tex2->num_srcs)
>>>>>>> +         return false;
>>>>>>> +      for (unsigned i = 0; i < tex1->num_srcs; i++) {
>>>>>>> +         if (tex1->src[i].src_type != tex2->src[i].src_type ||
>>>>>>> +             !nir_srcs_equal(tex1->src[i].src, tex2->src[i].src)) {
>>>>>>> +            return false;
>>>>>>> +         }
>>>>>>> +      }
>>>>>>> +
>>>>>>> +      if (tex1->coord_components != tex2->coord_components ||
>>>>>>> +          tex1->sampler_dim != tex2->sampler_dim ||
>>>>>>> +          tex1->is_array != tex2->is_array ||
>>>>>>> +          tex1->is_shadow != tex2->is_shadow ||
>>>>>>> +          tex1->is_new_style_shadow != tex2->is_new_style_shadow ||
>>>>>>> +          memcmp(tex1->const_offset, tex2->const_offset,
>>>>>>> +                 sizeof(tex1->const_offset)) != 0 ||
>>>>>>> +          tex1->component != tex2->component ||
>>>>>>> +         tex1->sampler_index != tex2->sampler_index ||
>>>>>>> +         tex1->sampler_array_size != tex2->sampler_array_size) {
>>>>>>
>>>>>> I think some of these fields are sometimes unused, so you need to
>>>>>> split them out and not check them if they're unused. For example,
>>>>>> is_new_style_shadow is only used when is_shadow is true, and component
>>>>>> isn't used if we're not doing a textureGather (?) op -- I don't
>>>>>> remember all the details. That, or we have to have a defined value for
>>>>>> these fields when they're unused and document that somewhere.
>>>>>
>>>>> Oh god, we don't zero-allocate structures.  This is going to be awful.
>>>>
>>>> No, with valgrind it's actually better... you get an error if you
>>>> don't know what you're doing rather than silently having it ignored.
>>>> These days, zero-allocating structures by default doesn't make much
>>>> sense any more.
>>>
>>> So, why should nir.c not be zero-filling those fields, when we zero fill
>>> the nir_dests and nir_srcs and and tex->sampler and tex->sampler_index
>>> and deref->base_offset?
>>
>> because in 99% of cases, that's what we want. To be clear, I'm not
>> against initializing stuff to some default value when it makes sense,
>> and in this case I'm fine with initializing all the sometimes-used
>> things to 0 and ensuring that they stay 0 to make comparing
>> tex_instr's less of a PITA. But I don't think we should be blindly
>> calling calloc()/rzalloc() on everything; if there's some field the
>> caller has to set, and there's no default value that 99% of callers
>> will want, don't set it and then let valgrind find any errors (or,
>> even better, pass it as a parameter to the constructor). And yes,
>> there are probably a few places where we initialize stuff when it's
>> not necessary -- that's a mistake on my part.
>
> What I most object to about not calling calloc/rzalloc is that it means
> you don't get to find the errors you've created until you happen to run
> under valgrind, since you almost always get zero-filled contents anyway.

That's true, although maybe it's just a sign that you aren't using
valgrind enough. But the alternative of basically making valgrind
useless and masking cases where you actually forgot to initialize
stuff seems a lot worse to me.