Re: [Mesa-dev] [PATCH v2] glsl: Expand matrix flip optimization pass to cover more cases.
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_transpos
Re: [Mesa-dev] [PATCH v2] glsl: Expand matrix flip optimization pass to cover more cases.
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
[Mesa-dev] [PATCH v2] glsl: Expand matrix flip optimization pass to cover more cases.
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;
- 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;
+
+ /* 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 {
+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_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_transpo
