-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/03/2011 01:17 AM, Ian Romanick wrote:
> 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;
> }
Scratch that. That's what I get for 1) editing commit message through
57 versions of a patch and 2) responding to review comments at 1am. The
code actually generated is:
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 = !if_to_cond_assign_then@2 && x;
...
There are no extra instructions emitted (with or without CSE) in the
unnested case.
In the nested case if_to_cond_assign_then@2 is live "too long" and may
cause extra register pressure. There may also be one extra instruction
generated because of the redundant '&& x'.
I've updated the commit messages (again) to reflect the real reality.
>> 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);
>>> + }
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk45npEACgkQX1gOwKyEAw/I9QCbBbv4LSNfsbqPfO7VqxH+BXuS
sgMAn1FVzB4vJ3ITrF6JF87QGi7S52Zt
=+MpS
-----END PGP SIGNATURE-----
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
