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

[Kenneth Graunke]

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();
}