On 3 May 2013 16:07, Ian Romanick <i...@freedesktop.org> wrote: > From: Ian Romanick <ian.d.roman...@intel.com> > > Right now the lower_clip_distance_visitor lowers variable indexing into > gl_ClipDistance into variable indexing into both the array > gl_ClipDistanceMESA and the vectors of that array. For example, > > gl_ClipDistance[i] = f; > > becomes > > gl_ClipDistanceMESA[i/4][i%4] = f; >
Technically it becomes gl_ClipDistanceMESA[i>>2][i&3]. Granted it's equivalent, but you might want to change your commit message to be more in line with what the lowering pass actually does. > > However, variable indexing into vectors using ir_dereference_array is > being removed. Instead, ir_expression with ir_triop_vector_insert will > be used. The above code will become > > gl_ClipDistanceMESA[i/4] = vector_insert(gl_ClipDistanceMESA[i/4], > i % 4, > f); > > In order to do this, an ir_rvalue_visitor will need to be used. This > commit is really just a refactor to get ready for that. > This commit is actually more than just a refactor. It also modifies how the lowering pass treats statements of the form: foo = gl_ClipDistance; so that instead of being transformed to: foo[0] = gl_ClipDistance[0][0]; foo[1] = gl_ClipDistance[0][1]; ... they are transformed to: foo[0] = gl_ClipDistance[0].x; foo[1] = gl_ClipDistance[0].y; I don't really understand why this is necessary (since a later optimization pass would make this transformation anyhow), and I believe that it introduces a bug (see below). I'd prefer to see this modification either dropped or split into its own patch, with an explanation of why it is necessary. > > v2: Convert tabs to spaces. Suggested by Eric. > > Signed-off-by: Ian Romanick <ian.d.roman...@intel.com> > Cc: Paul Berry <stereotype...@gmail.com> > --- > src/glsl/lower_clip_distance.cpp | 136 > +++++++++++++++++++++++++-------------- > 1 file changed, 86 insertions(+), 50 deletions(-) > > diff --git a/src/glsl/lower_clip_distance.cpp > b/src/glsl/lower_clip_distance.cpp > index 643807d..19068fb 100644 > --- a/src/glsl/lower_clip_distance.cpp > +++ b/src/glsl/lower_clip_distance.cpp > @@ -46,10 +46,10 @@ > */ > > #include "glsl_symbol_table.h" > -#include "ir_hierarchical_visitor.h" > +#include "ir_rvalue_visitor.h" > #include "ir.h" > > -class lower_clip_distance_visitor : public ir_hierarchical_visitor { > +class lower_clip_distance_visitor : public ir_rvalue_visitor { > public: > lower_clip_distance_visitor() > : progress(false), old_clip_distance_var(NULL), > @@ -59,11 +59,12 @@ public: > > virtual ir_visitor_status visit(ir_variable *); > void create_indices(ir_rvalue*, ir_rvalue *&, ir_rvalue *&); > - virtual ir_visitor_status visit_leave(ir_dereference_array *); > virtual ir_visitor_status visit_leave(ir_assignment *); > void visit_new_assignment(ir_assignment *ir); > virtual ir_visitor_status visit_leave(ir_call *); > > + virtual void handle_rvalue(ir_rvalue **rvalue); > + > bool progress; > > /** > @@ -173,33 +174,35 @@ > lower_clip_distance_visitor::create_indices(ir_rvalue *old_index, > } > > > -/** > - * Replace any expression that indexes into the gl_ClipDistance array > with an > - * expression that indexes into one of the vec4's in gl_ClipDistanceMESA > and > - * accesses the appropriate component. > - */ > -ir_visitor_status > -lower_clip_distance_visitor::visit_leave(ir_dereference_array *ir) > +void > +lower_clip_distance_visitor::handle_rvalue(ir_rvalue **rv) > { > /* If the gl_ClipDistance var hasn't been declared yet, then > * there's no way this deref can refer to it. > */ > - if (!this->old_clip_distance_var) > - return visit_continue; > - > - ir_dereference_variable *old_var_ref = > ir->array->as_dereference_variable(); > - if (old_var_ref && old_var_ref->var == this->old_clip_distance_var) { > - this->progress = true; > - ir_rvalue *array_index; > - ir_rvalue *swizzle_index; > - this->create_indices(ir->array_index, array_index, swizzle_index); > - void *mem_ctx = ralloc_parent(ir); > - ir->array = new(mem_ctx) ir_dereference_array( > - this->new_clip_distance_var, array_index); > - ir->array_index = swizzle_index; > + if (!this->old_clip_distance_var || *rv == NULL) > + return; > + > + ir_dereference_array *const array = (*rv)->as_dereference_array(); > Can we rename this var to "array_deref"? It's confusing to see the "array->array" below. > + if (array != NULL) { > + /* Replace any expression that indexes into the gl_ClipDistance > array > + * with an expression that indexes into one of the vec4's in > + * gl_ClipDistanceMESA and accesses the appropriate component. > + */ > + ir_dereference_variable *old_var_ref = > + array->array->as_dereference_variable(); > + if (old_var_ref && old_var_ref->var == this->old_clip_distance_var) > { > + this->progress = true; > + ir_rvalue *array_index; > + ir_rvalue *swizzle_index; > + this->create_indices(array->array_index, array_index, > swizzle_index); > + void *mem_ctx = ralloc_parent(array); > + array->array = > + new(mem_ctx) ir_dereference_array(this->new_clip_distance_var, > + array_index); > + array->array_index = swizzle_index; > + } > } > - > - return visit_continue; > } > > > @@ -214,38 +217,71 @@ > lower_clip_distance_visitor::visit_leave(ir_assignment *ir) > { > ir_dereference_variable *lhs_var = ir->lhs->as_dereference_variable(); > ir_dereference_variable *rhs_var = ir->rhs->as_dereference_variable(); > - if ((lhs_var && lhs_var->var == this->old_clip_distance_var) > - || (rhs_var && rhs_var->var == this->old_clip_distance_var)) { > - /* LHS or RHS of the assignment is the entire gl_ClipDistance array. > - * Since we are reshaping gl_ClipDistance from an array of floats > to an > - * array of vec4's, this isn't going to work as a bulk assignment > - * anymore, so unroll it to element-by-element assignments and lower > - * each of them. > - * > - * Note: to unroll into element-by-element assignments, we need to > make > - * clones of the LHS and RHS. This is only safe if the LHS and RHS > are > - * side-effect free. Fortunately, we know that they are, because > the > - * only kind of rvalue that can have side effects is an ir_call, and > - * ir_calls only appear (a) as a statement on their own, or (b) as > the > - * RHS of an assignment that stores the result of the call in a > - * temporary variable. > - */ > - void *ctx = ralloc_parent(ir); > + void *ctx = ralloc_parent(ir); > + > + /* LHS or RHS of the assignment is the entire gl_ClipDistance array. > + * Since we are reshaping gl_ClipDistance from an array of floats to an > + * array of vec4's, this isn't going to work as a bulk assignment > + * anymore, so unroll it to element-by-element assignments and lower > + * each of them. > + * > + * Note: to unroll into element-by-element assignments, we need to make > + * clones of the LHS and RHS. This is only safe if the LHS and RHS are > + * side-effect free. Fortunately, we know that they are, because the > + * only kind of rvalue that can have side effects is an ir_call, and > + * ir_calls only appear (a) as a statement on their own, or (b) as the > + * RHS of an assignment that stores the result of the call in a > + * temporary variable. > + */ > The last two sentences of this comment are out of date, since a long time ago we modified ir_call to be a statement rather than an expression. If you're going to move this comment, please change the last two sentences to something like "This is safe because expressions are side-effect free". > + if (lhs_var && lhs_var->var == this->old_clip_distance_var) { > int array_size = this->old_clip_distance_var->type->array_size(); > for (int i = 0; i < array_size; ++i) { > - ir_dereference_array *new_lhs = new(ctx) ir_dereference_array( > - ir->lhs->clone(ctx, NULL), new(ctx) ir_constant(i)); > - new_lhs->accept(this); > + ir_dereference_variable *const new_deref = > + new(ctx) ir_dereference_variable(new_clip_distance_var); > + > + ir_dereference_array *const new_lhs = > + new(ctx) ir_dereference_array(new_deref, > + new(ctx) ir_constant(i / 4)); > + > ir_dereference_array *new_rhs = new(ctx) ir_dereference_array( > ir->rhs->clone(ctx, NULL), new(ctx) ir_constant(i)); > - new_rhs->accept(this); > - this->base_ir->insert_before( > - new(ctx) ir_assignment(new_lhs, new_rhs)); > + > + const int mask = 1 << (i % 4); > + > + ir_assignment *const assign = > + new(ctx) ir_assignment(new_lhs, new_rhs, NULL, mask); > + > + this->base_ir->insert_before(assign); > } > ir->remove(); > + > + return visit_continue; > + } else if (rhs_var && rhs_var->var == this->old_clip_distance_var) { > + int array_size = this->old_clip_distance_var->type->array_size(); > + for (int i = 0; i < array_size; ++i) { > + ir_dereference_array *new_lhs = new(ctx) ir_dereference_array( > + ir->lhs->clone(ctx, NULL), new(ctx) ir_constant(i)); > + > + ir_dereference_variable *const new_deref = > + new(ctx) ir_dereference_variable(new_clip_distance_var); > + > + ir_rvalue *const new_rhs = > + new(ctx) ir_swizzle(new(ctx) ir_dereference_array(new_deref, > + new(ctx) > ir_constant(i / 4)), > + i % 4, 0, 0, 0, 1); > + > + ir_assignment *const assign = > + new(ctx) ir_assignment(new_lhs, new_rhs); > + > + this->base_ir->insert_before(assign); > + } > + ir->remove(); > + > + return visit_continue; > } > As I alluded to above, it's not clear to me why splitting this code into two separate cases (one for LHS, one for RHS) is necessary/beneficial. Also, I suspect that we now have a bug if the shader contains the (admittedly silly) statement: gl_ClipDistance = gl_ClipDistance; > > - return visit_continue; > + handle_rvalue((ir_rvalue **)&ir->lhs); > + return rvalue_visit(ir); > I could have really used a comment explaining why the call to handle_rvalue() is necessary here. Something like: "rvalue_visit(ir_assignment*) only visits the RHS of the assignment, but we need references to gl_ClipDistance in the LHS to be lowered also." } > > > @@ -330,7 +366,7 @@ lower_clip_distance_visitor::visit_leave(ir_call *ir) > } > } > > - return visit_continue; > + return rvalue_visit(ir); > } > > > -- > 1.8.1.4 > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev