-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/02/2011 07:45 PM, Eric Anholt wrote:
> On Tue, 2 Aug 2011 17:52:03 -0700, "Ian Romanick" <i...@freedesktop.org>
> wrote:
>> From: Ian Romanick <ian.d.roman...@intel.com>
>>
>> Now the condition (for the then-clause) and the inverse condition (for
>> the else-clause) get written to sepearate temporary variables. In the
>
> "separate"
>
>> presense of complex conditions, this may result in more code being
>> generated.
>
> I hope s/more/less/
More. If the original if-statement was
if (a && b && c && d && e) {
...
} else {
...
}
The lowered code will be
if_to_cond_assign_then = a && b && c && d && e;
...
if_to_cond_assign_else = !(a && b && c && d && e);
...
Of course, pretty much the only time I see complex conditions like this
in shaders is when one of our lowering passes does something stupid. :)
The assignments get generated in this way because of the way the
follow-on patch changes lowering of nested if-statements. In that
patch, code like
if (x) {
if (a && b && c && d && e) {
...
} else {
...
}
}
becomes
if_to_cond_assign_then@1 = x;
if_to_cond_assign_then@2 = a && b && c && d && e && x;
...
if_to_cond_assign_else@2 = !(a && b && c && d && e) && x;
...
I tried a couple of ways of generating the condition to a temporary, but
it got complex to manage. I basically was doing CSE by hand.
However, it now occurs to me that this change is completely bogus. Ugh.
The following code will do the wrong thing:
x = <some uniform containing the value 7>
if (x > 6) {
x = 5;
} else {
discard;
}
> And this is all because we don't CSE, right. We should get on that :)
Yes. That's the main reason I didn't put more time into optimal
management of complex conditions. Based on my above observation, it
seems I'll have to do that any way. Grr.
> Patch 1,2 get Reviewed-by: Eric Anholt <e...@anholt.net>
>
> Couple more comments below.
>
>> ---
>> src/glsl/lower_if_to_cond_assign.cpp | 62
>> ++++++++++++++++++++++-----------
>> 1 files changed, 41 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/glsl/lower_if_to_cond_assign.cpp
>> b/src/glsl/lower_if_to_cond_assign.cpp
>> index 5c74bc1..3f82700 100644
>> --- a/src/glsl/lower_if_to_cond_assign.cpp
>> +++ b/src/glsl/lower_if_to_cond_assign.cpp
>> @@ -136,7 +136,6 @@ ir_if_to_cond_assign_visitor::visit_leave(ir_if *ir)
>> return visit_continue;
>>
>> bool found_control_flow = false;
>> - ir_variable *cond_var;
>> ir_assignment *assign;
>> ir_dereference_variable *deref;
>>
>> @@ -154,31 +153,52 @@ ir_if_to_cond_assign_visitor::visit_leave(ir_if *ir)
>>
>> void *mem_ctx = ralloc_parent(ir);
>>
>> - /* Store the condition to a variable so the assignment conditions are
>> - * simpler.
>> + /* Store the condition to a variable. Move all of the instructions from
>> + * the then-clause if the if-statement. Use the condition variable as a
>
> "of the if-statement"
Right.
>> + * condition for all assignments.
>> */
>> - cond_var = new(mem_ctx) ir_variable(glsl_type::bool_type,
>> - "if_to_cond_assign_condition",
>> - ir_var_temporary);
>> - ir->insert_before(cond_var);
>> -
>> - deref = new(mem_ctx) ir_dereference_variable(cond_var);
>> - assign = new(mem_ctx) ir_assignment(deref,
>> - ir->condition, NULL);
>> + ir_variable *const then_var =
>> + new(mem_ctx) ir_variable(glsl_type::bool_type,
>> + "if_to_cond_assign_then",
>> + ir_var_temporary);
>> + ir->insert_before(then_var);
>> +
>> + ir_dereference_variable *then_cond =
>> + new(mem_ctx) ir_dereference_variable(then_var);
>> +
>> + assign = new(mem_ctx) ir_assignment(then_cond, ir->condition);
>> ir->insert_before(assign);
>>
>> - /* Now, move all of the instructions out of the if blocks, putting
>> - * conditions on assignments.
>> - */
>> - move_block_to_cond_assign(mem_ctx, ir, deref,
>> + move_block_to_cond_assign(mem_ctx, ir, then_cond,
>> &ir->then_instructions);
>>
>> - ir_rvalue *inverse =
>> - new(mem_ctx) ir_expression(ir_unop_logic_not,
>> - glsl_type::bool_type,
>> - deref->clone(mem_ctx, NULL),
>> - NULL);
>> - move_block_to_cond_assign(mem_ctx, ir, inverse, &ir->else_instructions);
>> + /* If there are instructions in the else-clause, store the inverse of the
>> + * condition to a variable. Move all of the instructions from the
>> + * else-clause if the if-statement. Use the (inverse) condition variable
>> + * as a condition for all assignments.
>> + */
>> + if (!ir->else_instructions.is_empty()) {
>> + ir_variable *const else_var =
>> + new(mem_ctx) ir_variable(glsl_type::bool_type,
>> + "if_to_cond_assign_else",
>> + ir_var_temporary);
>> + ir->insert_before(else_var);
>> +
>> + ir_dereference_variable *else_cond =
>> + new(mem_ctx) ir_dereference_variable(else_var);
>> +
>> + ir_rvalue *inverse =
>> + new(mem_ctx) ir_expression(ir_unop_logic_not,
>> + glsl_type::bool_type,
>> + then_cond->clone(mem_ctx, NULL),
>> + NULL);
>
> Could drop the bool_type on ir_expression()
Ah, right. I had copy-and-pasted some other code from this file that
predated that version of the constructor.
>> + assign = new(mem_ctx) ir_assignment(else_cond, inverse);
>> + ir->insert_before(assign);
>> +
>> + move_block_to_cond_assign(mem_ctx, ir, else_cond,
>> + &ir->else_instructions);
>> + }
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk45A/4ACgkQX1gOwKyEAw/ULwCfQFRNLTaJpP/d6Yl/XDYVBZb3
igoAnjeB5Bj+Q46AAf2ctXQl3YT0lHFU
=khjV
-----END PGP SIGNATURE-----
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
