[Mesa-dev] [PATCH 03/15] i965/cfg: Rework to make IF & ELSE blocks flow into ENDIF.

[Matt Turner]

On Wed, Dec 4, 2013 at 1:17 PM, Eric Anholt <eric at anholt.net> wrote:
> Matt Turner <mattst88 at gmail.com> writes:
>
>> Previously we made the basic block following an ENDIF instruction a
>> successor of the basic blocks ending with IF and ELSE. The PRM says that
>> IF and ELSE instructions jump *to* the ENDIF, rather than over it.
>>
>> This should be immaterial to dataflow analysis, except for if, break,
>> endif sequences:
>>
>>    START B1 <-B0 <-B9
>> 0x00000100: cmp.g.f0(8)     null            g15<8,8,1>F     g4<0,1,0>F
>> 0x00000110: (+f0) if(8) 0 0                 null            0x00000000UD
>>    END B1 ->B2 ->B4
>>    START B2 <-B1
>>    break
>> 0x00000120: break(8) 0 0                    null            0D
>>    END B2 ->B10
>>    START B3
>> 0x00000130: endif(8) 2                      null            0x00000002UD
>>    END B3 ->B4
>>
>> The ENDIF block would have no parents, so dataflow analysis would
>> generate incorrect results, preventing copy propagation from eliminating
>> some instructions.
>>
>> This patch changes the CFG to make ENDIF start rather than end basic
>> blocks, so that it can be the jump target of the IF and ELSE
>> instructions.
>>
>> It helps three programs (including two fs8/fs16 pairs) and hurts a
>> single fs8/fs16 pair.
>>
>> total instructions in shared programs: 1561126 -> 1561066 (-0.00%)
>> instructions in affected programs:     983 -> 923 (-6.10%)
>>
>> More importantly, it allows copy propagation to handle more cases.
>> Disabling the register_coalesce() pass before this patch hurts 58
>> programs, while afterward it only hurts 11 programs.
>> ---
>>  src/mesa/drivers/dri/i965/brw_cfg.cpp | 46 +++++++++++++++++++++++++----------
>>  1 file changed, 33 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.cpp b/src/mesa/drivers/dri/i965/brw_cfg.cpp
>> index 548b458..83c3c34 100644
>> --- a/src/mesa/drivers/dri/i965/brw_cfg.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_cfg.cpp
>> @@ -133,10 +133,7 @@ cfg_t::create(void *parent_mem_ctx, exec_list *instructions)
>>
>>        cur_if = cur;
>>        cur_else = NULL;
>> -      /* Set up the block just after the endif.  Don't know when exactly
>> -       * it will start, yet.
>> -       */
>> -      cur_endif = new_block();
>> +         cur_endif = NULL;
>>
>>        /* Set up our immediately following block, full of "then"
>>         * instructions.
>> @@ -149,26 +146,49 @@ cfg_t::create(void *parent_mem_ctx, exec_list *instructions)
>>        break;
>>
>>        case BRW_OPCODE_ELSE:
>> -      cur->add_successor(mem_ctx, cur_endif);
>> +         cur_else = cur;
>>
>>        next = new_block();
>>        next->start = (backend_instruction *)inst->next;
>>        cur_if->add_successor(mem_ctx, next);
>> -      cur_else = next;
>>
>>        set_next_block(next);
>>        break;
>>
>>        case BRW_OPCODE_ENDIF: {
>> -      cur_endif->start = (backend_instruction *)inst->next;
>> -      cur->add_successor(mem_ctx, cur_endif);
>> -      set_next_block(cur_endif);
>> +         backend_instruction *prev_inst = (backend_instruction *)inst->prev;
>> +         switch (prev_inst->opcode) {
>> +         case BRW_OPCODE_IF:
>> +         case BRW_OPCODE_ELSE:
>> +         case BRW_OPCODE_WHILE:
>> +         case BRW_OPCODE_DO:
>> +         case BRW_OPCODE_BREAK:
>> +         case BRW_OPCODE_CONTINUE:
>> +            /* New block was just created; use it. */
>> +            cur_endif = cur;
>> +            break;
>
> Instead of the opcode switch for this test, couldn't you just do "if
> cur->start == inst"?

Yes, that is better.

> Other than that, this series is:
>
> Reviewed-by: Eric Anholt <eric at anholt.net>

Thanks!