[Mesa-dev] [PATCH 3/4] glsl: Slight change to the code generated by if-flattening

[Ian Romanick]

-----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" <idr at freedesktop.org> wrote:
>> From: Ian Romanick <ian.d.romanick at 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 at 1 = x;
   if_to_cond_assign_then at 2 = a && b && c && d && e && x;
   ...
   if_to_cond_assign_else at 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 <eric at 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-----