[Mesa-dev] [PATCH 1/5] i965: Extend dead control flow to remove useless ELSE.

[Matt Turner]

On Wed, Jan 8, 2014 at 1:00 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On 01/08/2014 12:43 PM, Matt Turner wrote:
>> total instructions in shared programs: 1500212 -> 1500153 (-0.00%)
>> instructions in affected programs:     17777 -> 17718 (-0.33%)
>> ---
>>  src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
>> index 63a3e5b..82e83b8 100644
>> --- a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp
>> @@ -33,6 +33,7 @@
>>   *
>>   *   - if/endif
>>   *   - if/else/endif
>> + *   - .../else/endif
>
> Presumably the point of this is to change:
> IF, foo, bar, ELSE, ENDIF  ===>  IF, foo, bar, ENDIF
>
> ...but if I can read, that's not what this does at all...
>
>>   */
>>  bool
>>  dead_control_flow_eliminate(backend_visitor *v)
>> @@ -59,16 +60,17 @@ dead_control_flow_eliminate(backend_visitor *v)
>>           found = true;
>>        } else if (prev_inst->opcode == BRW_OPCODE_ELSE) {
>>           else_inst = prev_inst;
>> +         found = true;
>>
>>           prev_inst = (backend_instruction *) prev_inst->prev;
>>           if (prev_inst->opcode == BRW_OPCODE_IF) {
>>              if_inst = prev_inst;
>> -            found = true;
>>           }
>>        }
>>
>>        if (found) {
>> -         if_inst->remove();
>> +         if (if_inst)
>> +            if_inst->remove();
>>           if (else_inst)
>>              else_inst->remove();
>>           endif_inst->remove();
>>
>
> So in the above case...found = true, endif_inst != NULL, else_inst !=
> NULL, but if_inst == NULL.
>
> You delete ELSE, and you delete ENDIF.  You leave IF in place...OUCH.
>
> I think you could fix this by changing this to:
>
> if (else_inst)
>    else_inst->remove();
>
> if (if_inst) {
>     if_inst->remove();
>     endif_inst->remove();
> }

Yep, that does seem bogus. I'll send an updated patch.