On Thu, Dec 11, 2014 at 11:34:12PM +0100, Eduardo Lima Mitev wrote:
> From: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
>
> 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.
I wonder where the issue is addressed that you may have more elements than there
is space. The algorithm below doesn't write to invalid locations, but the
problem exists.
>
> This patch fixes the following dEQP test:
>
>
> dEQP-GLES3.functional.shaders.conversions.matrix_combine.float_bvec4_ivec2_bool_to_mat4x2_vertex
>
> No piglit regressions observed
>
> Signed-off-by: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
> ---
> src/glsl/ast_function.cpp | 121
> ++++++++++++++++++++++++++--------------------
> 1 file changed, 68 insertions(+), 53 deletions(-)
>
> diff --git a/src/glsl/ast_function.cpp b/src/glsl/ast_function.cpp
> index cbff9d8..451e3be 100644
> --- a/src/glsl/ast_function.cpp
> +++ b/src/glsl/ast_function.cpp
> @@ -1334,67 +1334,82 @@ emit_inline_matrix_constructor(const glsl_type *type,
> unsigned row_idx = 0;
>
> foreach_in_list(ir_rvalue, rhs, parameters) {
> - const unsigned components_remaining_this_column = rows - row_idx;
> + unsigned components_remaining_this_column;
> unsigned rhs_components = rhs->type->components();
> unsigned rhs_base = 0;
You may as well fix the whitespace here too.
>
> - /* 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);
> -
In the old code... if a >= b, then the MIN is just b, right?
> - 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;
> + /* 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 {
> + components_remaining_this_column = rows - row_idx;
> + /* 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 (components_remaining_this_column > 0 &&
> + (rhs_components - rhs_base) >=
> components_remaining_this_column) {
> + const unsigned count = MIN2(rhs_components - rhs_base,
> + components_remaining_this_column);
With the realization from the original, this too, isn't it just
components_remaining_this_column? That allows you to kill the count variable and
make the code a bit simpler (IMO). But instead...
I may be misunderstanding how the code is supported to work, but it looks to me
like the loop you added for assigning extra elements (rhs_components) belongs
here. As I understand it, the code is filling up space a row at a time, and then
moving over to the next column. It seems like if you add the do while loop, you
could get rid of the next if entirely - even the fist 'if' becomes just your
do_while loop.
Overgeneralized version in my head...
foreach_in_list {
i = 0;
do {
mat[col % height][row % width] = components[i++];
} while (i < (width * height))
}
>
> - col_idx++;
> - row_idx = 0;
> - }
> + rhs_var_ref = new(ctx) ir_dereference_variable(rhs_var);
>
> - /* 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;
> + ir_instruction *inst = assign_to_matrix_column(var, col_idx,
> + row_idx,
> + rhs_var_ref, 0,
> + count, ctx);
> + instructions->push_tail(inst);
>
> - rhs_var_ref = new(ctx) ir_dereference_variable(rhs_var);
> + rhs_base += count;
>
> - ir_instruction *inst = assign_to_matrix_column(var, col_idx,
> - row_idx,
> - rhs_var_ref,
> - rhs_base,
> - count, ctx);
> - instructions->push_tail(inst);
> + col_idx++;
> + row_idx = 0;
> + components_remaining_this_column = rows;
> + }
>
> - row_idx += count;
> - }
> + /* 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 = MIN2(components_remaining_this_column,
> + 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;
> + }
Add a space here
> + if (row_idx >= rows) {
> + row_idx = 0;
> + col_idx++;
> + }
And another one here. It seems like
> + /* Sometimes, there is still data left in the parameters and
> + * components left to be set in the destination but in other
> + * column. This loop makes sure that all the data that can be
> + * copied is actually copied.
> + */
> + } while(col_idx < cols && rhs_base < rhs_components);
> }
> }
>
AFAICT, your code is correct though. If you spend a few seconds and feel that
there really isn't a better way to implement this, you can consider it (with the
requested nitpicks):
Reviewed-by: Ben Widawsky <b...@bwidawsk.net>
If you do agree it can probably be improved, feel free to Cc me on the next rev,
and I will look at it.
--
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
