-----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 <i...@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" <i...@freedesktop.org>
>>> wrote:
>>>> From: Ian Romanick <ian.d.roman...@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-----
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
