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

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/22/2011 11:27 AM, Eric Anholt wrote:
> 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.

I only realized why I did it like that when I started converting it back. :)

I wanted this function and
ir_vec_index_to_cond_assign_visitor::visit_leave(ir_assignment *) to
look as much alike as possible.  The later function uses a list for
everything because it might base_ir->insert_before(list) or it might
wrap the list in an if-statement (in the case where there's already a
condition on the assignment).

I'd like to refactor that code to be more like the non-constant array
index lowering pass.  The r-value and l-value case only differ in the
order of the operands in the generated assignments and the optional
if-statement wrapping.  It really should be unified.

My inclination is to convert the one missed base_ir->insert_before to a
list.push_tail.  Does that see alright, or would you prefer it the other
way?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk4p95MACgkQX1gOwKyEAw8BgwCeNSgcW9tYm94PcV1Ft/r4Qm9T
ZO8AniydP7UJEHPkGcpgDMA5HZ+fi+Se
=XIcM
-----END PGP SIGNATURE-----