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 <[email protected]> > > > > 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 <[email protected]> > > --- > > > > 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 <[email protected]> > > 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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
