[Mesa-dev] [PATCH 08/12] nir/types: Make glsl_get_length smarter
Rob Clark
robdclark at gmail.com
Fri Apr 17 06:03:15 PDT 2015
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)..
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