[Mesa-dev] [PATCH v2] glsl: fix assignment of multiple scalar and vecs to matrices.

[Ben Widawsky]

On Tue, Apr 07, 2015 at 03:49:55PM +0200, Samuel Iglesias Gonsalvez wrote:
> When a vec has more elements than row components in a matrix, the
> code could end up failing an assert inside assign_to_matrix_column().
> 
> This patch makes sure that when there is still room in the matrix for
> more elements (but in other columns of the matrix), the data is actually
> assigned.
> 
> This patch fixes the following dEQP test:
> 
>   dEQP-GLES3.functional.shaders.conversions.matrix_combine.float_bvec4_ivec2_bool_to_mat4x2_vertex
>   dEQP-GLES3.functional.shaders.conversions.matrix_combine.float_bvec4_ivec2_bool_to_mat4x2_fragment
> 
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
> ---
> 
> v2:
>    - Improve the patch following Ben's comments.
> 
>  src/glsl/ast_function.cpp | 110 +++++++++++++++++++++-------------------------
>  1 file changed, 49 insertions(+), 61 deletions(-)
> 
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index 918be69..0010ffe 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -1370,71 +1370,59 @@ emit_inline_matrix_constructor(const glsl_type *type,
>     } else {
>        const unsigned cols = type->matrix_columns;
>        const unsigned rows = type->vector_elements;
> +      unsigned remaining_slots = rows * cols;
>        unsigned col_idx = 0;
>        unsigned row_idx = 0;
>  
>        foreach_in_list(ir_rvalue, rhs, parameters) {
> -	 const unsigned components_remaining_this_column = rows - row_idx;
> -	 unsigned rhs_components = rhs->type->components();
> -	 unsigned rhs_base = 0;
> -
> -	 /* Since the parameter might be used in the RHS of two assignments,
> -	  * generate a temporary and copy the paramter there.
> -	  */
> -	 ir_variable *rhs_var =
> -	    new(ctx) ir_variable(rhs->type, "mat_ctor_vec", ir_var_temporary);
> -	 instructions->push_tail(rhs_var);
> -
> -	 ir_dereference *rhs_var_ref =
> -	    new(ctx) ir_dereference_variable(rhs_var);
> -	 ir_instruction *inst = new(ctx) ir_assignment(rhs_var_ref, rhs, NULL);
> -	 instructions->push_tail(inst);
> -
> -	 /* Assign the current parameter to as many components of the matrix
> -	  * as it will fill.
> -	  *
> -	  * NOTE: A single vector parameter can span two matrix columns.  A
> -	  * single vec4, for example, can completely fill a mat2.
> -	  */
> -	 if (rhs_components >= components_remaining_this_column) {
> -	    const unsigned count = MIN2(rhs_components,
> -					components_remaining_this_column);
> -
> -	    rhs_var_ref = new(ctx) ir_dereference_variable(rhs_var);
> -
> -	    ir_instruction *inst = assign_to_matrix_column(var, col_idx,
> -							   row_idx,
> -							   rhs_var_ref, 0,
> -							   count, ctx);
> -	    instructions->push_tail(inst);
> -
> -	    rhs_base = count;
> -
> -	    col_idx++;
> -	    row_idx = 0;
> -	 }
> -
> -	 /* If there is data left in the parameter and components left to be
> -	  * set in the destination, emit another assignment.  It is possible
> -	  * that the assignment could be of a vec4 to the last element of the
> -	  * matrix.  In this case col_idx==cols, but there is still data
> -	  * left in the source parameter.  Obviously, don't emit an assignment
> -	  * to data outside the destination matrix.
> -	  */
> -	 if ((col_idx < cols) && (rhs_base < rhs_components)) {
> -	    const unsigned count = rhs_components - rhs_base;
> -
> -	    rhs_var_ref = new(ctx) ir_dereference_variable(rhs_var);
> -
> -	    ir_instruction *inst = assign_to_matrix_column(var, col_idx,
> -							   row_idx,
> -							   rhs_var_ref,
> -							   rhs_base,
> -							   count, ctx);
> -	    instructions->push_tail(inst);
> -
> -	    row_idx += count;
> -	 }
> +         unsigned rhs_components = rhs->type->components();
> +         unsigned rhs_base = 0;
> +
> +         if (remaining_slots == 0)
> +            break;
> +
> +         /* Since the parameter might be used in the RHS of two assignments,
> +          * generate a temporary and copy the paramter there.
> +          */
> +         ir_variable *rhs_var =
> +            new(ctx) ir_variable(rhs->type, "mat_ctor_vec", ir_var_temporary);
> +         instructions->push_tail(rhs_var);
> +
> +         ir_dereference *rhs_var_ref =
> +            new(ctx) ir_dereference_variable(rhs_var);
> +         ir_instruction *inst = new(ctx) ir_assignment(rhs_var_ref, rhs, NULL);
> +         instructions->push_tail(inst);
> +
> +         do {
> +            /* Assign the current parameter to as many components of the matrix
> +             * as it will fill.
> +             *
> +             * NOTE: A single vector parameter can span two matrix columns.  A
> +             * single vec4, for example, can completely fill a mat2.
> +             */
> +            unsigned count = MIN2(rows - row_idx,
> +                                  rhs_components - rhs_base);
> +
> +            rhs_var_ref = new(ctx) ir_dereference_variable(rhs_var);
> +            ir_instruction *inst = assign_to_matrix_column(var, col_idx,
> +                                                         row_idx,
> +                                                         rhs_var_ref,
> +                                                         rhs_base,
> +                                                         count, ctx);
> +            instructions->push_tail(inst);
> +            rhs_base += count;
> +            row_idx += count;
> +            remaining_slots -= count;
> +
> +            /* Sometimes, there is still data left in the parameters and
> +             * components left to be set in the destination but in other
> +             * column.
> +             */
> +            if (row_idx >= rows) {
> +               row_idx = 0;
> +               col_idx++;
> +            }
> +         } while(remaining_slots > 0 && rhs_base < rhs_components);
>        }
>     }
>  
> -- 
> 2.1.0

lgtm; though I didn't look as hard as I did in v1.

Reviewed-by: Ben Widawsky <ben at bwidawsk.net>


-- 
Ben Widawsky, Intel Open Source Technology Center