[Mesa-dev] [PATCH 06/16] glsl: fix assignment of multiple scalar and vecs to matrices.
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Mon Mar 30 04:38:00 PDT 2015
On Friday 27 March 2015 20:46:38 Ben Widawsky wrote:
> On Thu, Dec 11, 2014 at 11:34:12PM +0100, Eduardo Lima Mitev wrote:
> > From: Samuel Iglesias Gonsalvez <siglesias at 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_ive
> > c2_bool_to_mat4x2_vertex>
> > No piglit regressions observed
> >
> > Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at 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 <ben at 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.
Yeah, I will look at it after holidays (i.e. next week). I also think it can
be improved.
Thanks for your review,
Sam
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150330/3f1cb4e3/attachment-0001.sig>
More information about the mesa-dev
mailing list