On Wed, Dec 4, 2013 at 1:17 PM, Eric Anholt <e...@anholt.net> wrote:
> Matt Turner <matts...@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 <e...@anholt.net>
Thanks!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
