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

[Eric Anholt]

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"?

Other than that, this series is:

Reviewed-by: Eric Anholt <eric at anholt.net>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131204/6c8516d4/attachment.pgp>