[Mesa-dev] [PATCH 08/12] nir/types: Make glsl_get_length smarter

[Jason Ekstrand]

On Fri, Apr 17, 2015 at 6:06 AM, Rob Clark <robdclark at gmail.com> wrote:
> On Fri, Apr 17, 2015 at 9:03 AM, Rob Clark <robdclark at gmail.com> wrote:
>> On Fri, Apr 10, 2015 at 8:48 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>>> Previously, this function returned the number of elements for structures
>>> and arrays and 0 for everything else.  In NIR, this is almost never what
>>> you want because we also treat matricies as arrays so you have to
>>> special-case constantly.  We already had a helper for this in two places
>>> and should have had one in lower_vars_to_ssa.  We might as well make
>>> glsl_get_length just do the right thing in the first place.
>>>
>>> This also fixes a bug in locals_to_regs caused by not checking for the
>>> matrix case in one place.
>>
>> so, as this is, it blows up ttn, because all of a sudden vec4's become
>> length 4 instead of 0.. see below
>>
>>
>>> ---
>>>  src/glsl/nir/nir_lower_locals_to_regs.c | 11 ++---------
>>>  src/glsl/nir/nir_lower_var_copies.c     | 24 ++----------------------
>>>  src/glsl/nir/nir_lower_vars_to_ssa.c    | 26 +++-----------------------
>>>  src/glsl/nir/nir_types.cpp              | 16 +++++++++++++++-
>>>  4 files changed, 22 insertions(+), 55 deletions(-)
>>>

[snip]

>>> diff --git a/src/glsl/nir/nir_types.cpp b/src/glsl/nir/nir_types.cpp
>>> index f0d0b46..10be6df 100644
>>> --- a/src/glsl/nir/nir_types.cpp
>>> +++ b/src/glsl/nir/nir_types.cpp
>>> @@ -103,7 +103,21 @@ glsl_get_matrix_columns(const struct glsl_type *type)
>>>  unsigned
>>>  glsl_get_length(const struct glsl_type *type)
>>>  {
>>> -   return type->length;
>>> +   switch (glsl_get_base_type(type)) {
>>> +   case GLSL_TYPE_STRUCT:
>>> +   case GLSL_TYPE_ARRAY:
>>> +      return type->length;
>>> +   case GLSL_TYPE_FLOAT:
>>> +   case GLSL_TYPE_INT:
>>> +   case GLSL_TYPE_UINT:
>>> +   case GLSL_TYPE_BOOL:
>>> +      if (type->is_matrix())
>>> +         return type->matrix_columns;
>>> +      else
>>> +         return type->vector_elements;
>>
>> So I *think* you want to return 0 instead of type->vector_elements..
>> either that or for a matrix you would want matrix_columns *
>> vector_elements?  It at least seems inconsistent for a vector to
>> return number of elements, but a matrix to only return number of
>> vectors

The idea was to return whatever number of "thing"s the type has.  From
NIR's perspective, since we don't have a matrix type, a matrix is a
vec4[4] so it has 4 things.  An array has the length number of things,
a structure has field elements, and a vector has components.  The
reason for this last one is that, if we ever want to scalarize
load/store operations, we will need vectors to also have a length.
Even there, how does it make sense for a vec4 to have a length of 0?
Should it be at least 1?

>> I'm not quite sure what you expect the array length to be for vecN's..
>> it's possible that we need to adjust expectations in ttn (and vc4 and
>> freedreno/ir3)..
>
> oh, I should add that with this locally changed to return 0 instead of
> vector_elements, on top of your other variable-indexing patches: 11
> fail->pass out of variable-indexing tests..

Glad to hear it!
--Jason