[Mesa-dev] [PATCH 13/13] glsl: Compare vector indices in blocks

[Eric Anholt]

On Fri, 22 Jul 2011 09:27:36 -0700, Ian Romanick <idr at freedesktop.org> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 07/21/2011 05:28 PM, Eric Anholt wrote:
> > On Thu, 21 Jul 2011 12:16:58 -0700, "Ian Romanick" <idr at freedesktop.org> wrote:
> >> From: Ian Romanick <ian.d.romanick at intel.com>
> >>
> >> Just like the non-constant array index lowering pass, compare all N
> >> indices at once.  For accesses to a vec4, this saves 3 comparison
> >> instructions on a vector architecture.
> >> ---
> >>  src/glsl/lower_vec_index_to_cond_assign.cpp |   57 ++++++++++++++++----------
> >>  1 files changed, 35 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/src/glsl/lower_vec_index_to_cond_assign.cpp b/src/glsl/lower_vec_index_to_cond_assign.cpp
> >> index 15992e2..26c5452 100644
> >> --- a/src/glsl/lower_vec_index_to_cond_assign.cpp
> >> +++ b/src/glsl/lower_vec_index_to_cond_assign.cpp
> >> @@ -93,32 +93,40 @@ ir_vec_index_to_cond_assign_visitor::convert_vec_index_to_cond_assign(ir_rvalue
> >>     base_ir->insert_before(index);
> >>     deref = new(base_ir) ir_dereference_variable(index);
> >>     assign = new(base_ir) ir_assignment(deref, orig_deref->array_index, NULL);
> >> -   base_ir->insert_before(assign);
> >> +   list.push_tail(assign);
> >>  
> >>     /* Temporary where we store whichever value we swizzle out. */
> >>     var = new(base_ir) ir_variable(ir->type, "vec_index_tmp_v",
> >>  				  ir_var_temporary);
> >> -   base_ir->insert_before(var);
> >> +   list.push_tail(var);
> >> +
> >> +   /* Generate a single comparison condition "mask" for all of the components
> >> +    * in the vector.
> >> +    */
> >> +   ir_rvalue *const cond_deref =
> >> +      compare_index_block(&list, index, 0,
> >> +			  orig_deref->array->type->vector_elements,
> >> +			  mem_ctx);
> >>  
> >>     /* Generate a conditional move of each vector element to the temp. */
> >>     for (i = 0; i < orig_deref->array->type->vector_elements; i++) {
> >> -      deref = new(base_ir) ir_dereference_variable(index);
> >> -      condition = new(base_ir) ir_expression(ir_binop_equal,
> >> -					     glsl_type::bool_type,
> >> -					     deref,
> >> -					     new(base_ir) ir_constant(i));
> >> +      ir_rvalue *condition_swizzle =
> >> +	 new(ir) ir_swizzle(cond_deref->clone(ir, NULL), i, 0, 0, 0, 1);
> > 
> > If you've got the mem_ctx = ralloc_parent(base_ir) around already, it seems
> > like the right thing to use here (and in a few other places in this code)
> 
> That was something odd about the original code.  My addition of
> 'new(ir)' was a copy-and-paste mistake.  I had intended it to be
> 'new(base_ir)' to follow the original code.  But yeah.  Either mem_ctx
> should be removed, it should be used everywhere, or there should be a
> comment explaining why the code is weird.
> 
> For the purpose of this commit, I'll just change the 'new(ir)' to
> 'new(base_ir)'.
> 
> > Other than that, switching the base_ir->insert_before(whatever) to
> > list.push_tail() in various places (but not everywhere) seems strange.
> 
> The only instance of that is to put the whole set of new instructions in
> the IR stream.  I'll add a comment.
> 
> The reason it uses list.push_tail is because compare_index_block wants a
> list as a parameter, so there's a list for at least some of the added
> instructions.

I understood the temporary list thing, but what I found strange was this
hunk:

> @@ -93,32 +93,40 @@ ir_vec_index_to_cond_assign_visitor::convert_vec_index_to_cond_assign(ir_rvalue
>     base_ir->insert_before(index);
>     deref = new(base_ir) ir_dereference_variable(index);
>     assign = new(base_ir) ir_assignment(deref, orig_deref->array_index, NULL);
> -   base_ir->insert_before(assign);
> +   list.push_tail(assign);
>  
>     /* Temporary where we store whichever value we swizzle out. */
>     var = new(base_ir) ir_variable(ir->type, "vec_index_tmp_v",
>  				  ir_var_temporary);
> -   base_ir->insert_before(var);
> +   list.push_tail(var);

This appears to be a no-op to me.

I would have thought you would make the temp list, call the function
that generates into the list, and push the list, then leave the code
surrounding the new function call untouched and using
base_ir->insert_before as it used to.  Particularly when in this hunk
there's a base_ir->insert_before() left around right in the hunk, so
it's not even a complete conversion.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20110722/ab8efd37/attachment.pgp>