On Wed, 2014-06-18 at 14:43 -0700, Ian Romanick wrote: > On 06/18/2014 05:03 AM, Iago Toral Quiroga wrote: > > Also, as suggested by Ian Romanick, make it so we don't need a bunch of > > individual handles to flippable matrices, instead we register > > matrix/transpose_matrix pairs in a hash table for all built-in matrices > > using the non-transpose matrix name as key. > > --- > > src/glsl/opt_flip_matrices.cpp | 145 > > ++++++++++++++++++++++++++++++----------- > > 1 file changed, 108 insertions(+), 37 deletions(-) > > > > diff --git a/src/glsl/opt_flip_matrices.cpp b/src/glsl/opt_flip_matrices.cpp > > index 9044fd6..d0b8125 100644 > > --- a/src/glsl/opt_flip_matrices.cpp > > +++ b/src/glsl/opt_flip_matrices.cpp > > @@ -29,44 +29,132 @@ > > * On some hardware, this is more efficient. > > * > > * This currently only does the conversion for built-in matrices which > > - * already have transposed equivalents. Namely, > > gl_ModelViewProjectionMatrix > > - * and gl_TextureMatrix. > > + * already have transposed equivalents. > > */ > > #include "ir.h" > > #include "ir_optimization.h" > > #include "main/macros.h" > > +#include "program/hash_table.h" > > > > namespace { > > + > > class matrix_flipper : public ir_hierarchical_visitor { > > public: > > + struct matrix_and_transpose { > > + ir_variable *matrix; > > + ir_variable *transpose_matrix; > > + }; > > + > > matrix_flipper(exec_list *instructions) > > { > > progress = false; > > - mvp_transpose = NULL; > > - texmat_transpose = NULL; > > + > > + /* Build a hash table of built-in matrices and their transposes. > > + * > > + * The key for the entries in the hash table is the non-transpose > > matrix > > + * name. This assumes that all built-in transpose matrices have the > > + * "Transpose" suffix. > > + */ > > + ht = hash_table_ctor(0, hash_table_string_hash, > > + hash_table_string_compare); > > > > foreach_list(n, instructions) { > > ir_instruction *ir = (ir_instruction *) n; > > ir_variable *var = ir->as_variable(); > > - if (!var) > > + > > + /* Must be a matrix */ > > + if (!var || !var->type->is_matrix()) > > continue; > > gl_TextureMatrix is an array of matrices, so var->type->is_matrix() > will fail. I think you want: > > if (!var) > continue; > > /* Must be a matrix or array of matrices. */ > if (!var->type->is_matrix() && > !(var->type->is_array() && var->type->fields.array->is_matrix())) > continue;
Oh, right. > > - if (strcmp(var->name, "gl_ModelViewProjectionMatrixTranspose") == > > 0) > > - mvp_transpose = var; > > - if (strcmp(var->name, "gl_TextureMatrixTranspose") == 0) > > - texmat_transpose = var; > > + /* Must be a built-in */ > > + if (strstr(var->name, "gl_") != var->name) > > + continue; > > The name has to start with gl_, not just contain it. Use > is_gl_identifier(var->name) instead. Actually, this checks that it starts with it (see != var->name), but I'll use is_gl_identifier. > > + > > + /* Create a new entry for this matrix if we don't have one yet */ > > + bool new_entry = false; > > + struct matrix_and_transpose *entry = > > + (struct matrix_and_transpose *) hash_table_find(ht, var->name); > > + if (!entry) { > > + new_entry = true; > > + entry = new struct matrix_and_transpose(); > > + entry->matrix = NULL; > > + entry->transpose_matrix = NULL; > > + } > > + > > + const char *transpose_ptr = strstr(var->name, "Transpose"); > > + if (transpose_ptr == NULL) { > > + entry->matrix = var; > > + } else { > > It's probably worth adding an assertion in case a built-in is ever > added with something after Transpose. The probability is very, very > low, but I'd rather be safe. Sure, I will add that. > assert(transpose_ptr[9] == 0); > > > + entry->transpose_matrix = var; > > + } > > + > > + if (new_entry) { > > + char *entry_key; > > + if (transpose_ptr == NULL) { > > + entry_key = strdup(var->name); > > + } else { > > + entry_key = strndup(var->name, transpose_ptr - var->name); > > + } > > hash_table_dtor doesn't free the keys, so all of this memory leaks. > Use ralloc_strndup, and only copy the name in the transpose_ptr != NULL > case. Ok. > > + hash_table_insert(ht, entry, entry_key); > > + } > > } > > } > > > > + ~matrix_flipper() > > + { > > + hash_table_dtor(ht); > > + } > > + > > ir_visitor_status visit_enter(ir_expression *ir); > > > > bool progress; > > > > private: > > - ir_variable *mvp_transpose; > > - ir_variable *texmat_transpose; > > + void transform_operands(ir_expression *ir, > > + ir_variable *mat_var, > > + ir_variable *mat_transpose); > > + void transform_operands_array_of_matrix(ir_expression *ir, > > + ir_variable *mat_var, > > + ir_variable *mat_transpose); > > + struct hash_table *ht; > > }; > > } > > > > +void > > +matrix_flipper::transform_operands(ir_expression *ir, > > + ir_variable *mat_var, > > + ir_variable *mat_transpose) > > +{ > > +#ifndef NDEBUG > > + ir_dereference_variable *deref = > > ir->operands[0]->as_dereference_variable(); > > + assert(deref && deref->var == mat_var); > > +#endif > > + > > + void *mem_ctx = ralloc_parent(ir); > > + ir->operands[0] = ir->operands[1]; > > + ir->operands[1] = new(mem_ctx) ir_dereference_variable(mat_transpose); > > +} > > + > > +void > > +matrix_flipper::transform_operands_array_of_matrix(ir_expression *ir, > > + ir_variable *mat_var, > > + ir_variable > > *mat_transpose) > > +{ > > + ir_dereference_array *array_ref = > > ir->operands[0]->as_dereference_array(); > > + assert(array_ref != NULL); > > + ir_dereference_variable *var_ref = > > + array_ref->array->as_dereference_variable(); > > + assert(var_ref && var_ref->var == mat_var); > > + > > + ir->operands[0] = ir->operands[1]; > > + ir->operands[1] = array_ref; > > + > > + var_ref->var = mat_transpose; > > + > > + mat_transpose->data.max_array_access = > > + MAX2(mat_transpose->data.max_array_access, > > + mat_var->data.max_array_access); > > +} > > + > > ir_visitor_status > > matrix_flipper::visit_enter(ir_expression *ir) > > { > > @@ -79,34 +167,17 @@ matrix_flipper::visit_enter(ir_expression *ir) > > if (!mat_var) > > return visit_continue; > > > > - if (mvp_transpose && > > - strcmp(mat_var->name, "gl_ModelViewProjectionMatrix") == 0) { > > -#ifndef NDEBUG > > - ir_dereference_variable *deref = > > ir->operands[0]->as_dereference_variable(); > > - assert(deref && deref->var == mat_var); > > -#endif > > - > > - void *mem_ctx = ralloc_parent(ir); > > - > > - ir->operands[0] = ir->operands[1]; > > - ir->operands[1] = new(mem_ctx) > > ir_dereference_variable(mvp_transpose); > > + struct matrix_and_transpose *entry = > > + (struct matrix_and_transpose *) hash_table_find(ht, mat_var->name); > > + if (!entry || !entry->transpose_matrix) > > + return visit_continue; > > > > + if (strcmp(mat_var->name, "gl_TextureMatrix") == 0 || > > + strcmp(mat_var->name, "gl_TextureMatrixInverse") == 0) { > > + transform_operands_array_of_matrix(ir, mat_var, > > entry->transpose_matrix); > > progress = true; > > - } else if (texmat_transpose && > > - strcmp(mat_var->name, "gl_TextureMatrix") == 0) { > > - ir_dereference_array *array_ref = > > ir->operands[0]->as_dereference_array(); > > - assert(array_ref != NULL); > > - ir_dereference_variable *var_ref = > > array_ref->array->as_dereference_variable(); > > - assert(var_ref && var_ref->var == mat_var); > > - > > - ir->operands[0] = ir->operands[1]; > > - ir->operands[1] = array_ref; > > - > > - var_ref->var = texmat_transpose; > > - > > - texmat_transpose->data.max_array_access = > > - MAX2(texmat_transpose->data.max_array_access, > > mat_var->data.max_array_access); > > - > > + } else { > > + transform_operands(ir, mat_var, entry->transpose_matrix); > > progress = true; > > } > > > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev