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

Rob Clark robdclark at gmail.com
Fri Apr 17 06:06:51 PDT 2015


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(-)
>>
>> diff --git a/src/glsl/nir/nir_lower_locals_to_regs.c b/src/glsl/nir/nir_lower_locals_to_regs.c
>> index 8c5df7b..8c1977a 100644
>> --- a/src/glsl/nir/nir_lower_locals_to_regs.c
>> +++ b/src/glsl/nir/nir_lower_locals_to_regs.c
>> @@ -100,15 +100,8 @@ get_reg_for_deref(nir_deref_var *deref, struct locals_to_regs_state *state)
>>     unsigned array_size = 1;
>>     nir_deref *tail = &deref->deref;
>>     while (tail->child) {
>> -      if (tail->child->deref_type == nir_deref_type_array) {
>> -         /* Multiply by the parent's type. */
>> -         if (glsl_type_is_matrix(tail->type)) {
>> -            array_size *= glsl_get_matrix_columns(tail->type);
>> -         } else {
>> -            assert(glsl_get_length(tail->type) > 0);
>> -            array_size *= glsl_get_length(tail->type);
>> -         }
>> -      }
>> +      if (tail->child->deref_type == nir_deref_type_array)
>> +         array_size *= glsl_get_length(tail->type);
>>        tail = tail->child;
>>     }
>>
>> diff --git a/src/glsl/nir/nir_lower_var_copies.c b/src/glsl/nir/nir_lower_var_copies.c
>> index 58389a7..2167290 100644
>> --- a/src/glsl/nir/nir_lower_var_copies.c
>> +++ b/src/glsl/nir/nir_lower_var_copies.c
>> @@ -64,26 +64,6 @@ get_deref_tail(nir_deref *deref)
>>     return deref;
>>  }
>>
>> -static int
>> -type_get_length(const struct glsl_type *type)
>> -{
>> -   switch (glsl_get_base_type(type)) {
>> -   case GLSL_TYPE_STRUCT:
>> -   case GLSL_TYPE_ARRAY:
>> -      return glsl_get_length(type);
>> -   case GLSL_TYPE_FLOAT:
>> -   case GLSL_TYPE_INT:
>> -   case GLSL_TYPE_UINT:
>> -   case GLSL_TYPE_BOOL:
>> -      if (glsl_type_is_matrix(type))
>> -         return glsl_get_matrix_columns(type);
>> -      else
>> -         return glsl_get_vector_elements(type);
>> -   default:
>> -      unreachable("Invalid deref base type");
>> -   }
>> -}
>> -
>>  /* This function recursively walks the given deref chain and replaces the
>>   * given copy instruction with an equivalent sequence load/store
>>   * operations.
>> @@ -121,9 +101,9 @@ emit_copy_load_store(nir_intrinsic_instr *copy_instr,
>>        nir_deref_array *src_arr = nir_deref_as_array(src_arr_parent->child);
>>        nir_deref_array *dest_arr = nir_deref_as_array(dest_arr_parent->child);
>>
>> -      unsigned length = type_get_length(src_arr_parent->type);
>> +      unsigned length = glsl_get_length(src_arr_parent->type);
>>        /* The wildcards should represent the same number of elements */
>> -      assert(length == type_get_length(dest_arr_parent->type));
>> +      assert(length == glsl_get_length(dest_arr_parent->type));
>>        assert(length > 0);
>>
>>        /* Walk over all of the elements that this wildcard refers to and
>> diff --git a/src/glsl/nir/nir_lower_vars_to_ssa.c b/src/glsl/nir/nir_lower_vars_to_ssa.c
>> index 6aa3a79..4af90de 100644
>> --- a/src/glsl/nir/nir_lower_vars_to_ssa.c
>> +++ b/src/glsl/nir/nir_lower_vars_to_ssa.c
>> @@ -90,32 +90,12 @@ struct lower_variables_state {
>>     struct hash_table *phi_table;
>>  };
>>
>> -static int
>> -type_get_length(const struct glsl_type *type)
>> -{
>> -   switch (glsl_get_base_type(type)) {
>> -   case GLSL_TYPE_STRUCT:
>> -   case GLSL_TYPE_ARRAY:
>> -      return glsl_get_length(type);
>> -   case GLSL_TYPE_FLOAT:
>> -   case GLSL_TYPE_INT:
>> -   case GLSL_TYPE_UINT:
>> -   case GLSL_TYPE_BOOL:
>> -      if (glsl_type_is_matrix(type))
>> -         return glsl_get_matrix_columns(type);
>> -      else
>> -         return glsl_get_vector_elements(type);
>> -   default:
>> -      unreachable("Invalid deref base type");
>> -   }
>> -}
>> -
>>  static struct deref_node *
>>  deref_node_create(struct deref_node *parent,
>>                    const struct glsl_type *type, void *shader)
>>  {
>>     size_t size = sizeof(struct deref_node) +
>> -                 type_get_length(type) * sizeof(struct deref_node *);
>> +                 glsl_get_length(type) * sizeof(struct deref_node *);
>>
>>     struct deref_node *node = rzalloc_size(shader, size);
>>     node->type = type;
>> @@ -165,7 +145,7 @@ get_deref_node(nir_deref_var *deref, struct lower_variables_state *state)
>>        case nir_deref_type_struct: {
>>           nir_deref_struct *deref_struct = nir_deref_as_struct(tail);
>>
>> -         assert(deref_struct->index < type_get_length(node->type));
>> +         assert(deref_struct->index < glsl_get_length(node->type));
>>
>>           if (node->children[deref_struct->index] == NULL)
>>              node->children[deref_struct->index] =
>> @@ -184,7 +164,7 @@ get_deref_node(nir_deref_var *deref, struct lower_variables_state *state)
>>               * out-of-bounds offset.  We need to handle this at least
>>               * somewhat gracefully.
>>               */
>> -            if (arr->base_offset >= type_get_length(node->type))
>> +            if (arr->base_offset >= glsl_get_length(node->type))
>>                 return NULL;
>>
>>              if (node->children[arr->base_offset] == NULL)
>> 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
>
> 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..

BR,
-R

>
> BR,
> -R
>
>
>> +   default:
>> +      unreachable("Invalid base type");
>> +   }
>>  }
>>
>>  const char *
>> --
>> 2.3.5
>>
>> _______________________________________________
>> 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