[Mesa-dev] [PATCH v2] glsl: Expand matrix flip optimization pass to cover more cases.

[Ian Romanick]

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;

> -         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.

> +
> +         /* 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.

               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.

> +            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;
>     }
>