[Mesa-dev] [PATCH 10.5/15] i965/fs: Extend SEL peephole to handle only matching MOVs.
Paul Berry
stereotype441 at gmail.com
Wed Oct 30 18:28:53 CET 2013
On 29 October 2013 23:37, Matt Turner <mattst88 at gmail.com> wrote:
> Before this patch, the following code would not be optimized even though
> the final two instructions were common to the then and else blocks:
>
> (+f0) IF
> MOV dst2 ...
> MOV dst1 ...
> MOV dst0 ...
> ELSE
> MOV dst3 ...
> MOV dst1 ...
> MOV dst0 ...
> ENDIF
>
> This commit extends the peephole to handle this case.
>
> total instructions in shared programs: 1362275 -> 1361817 (-0.03%)
> instructions in affected programs: 35527 -> 35069 (-1.29%)
> ---
> Split mistakenly squashed patches.
>
> src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp | 93
> +++++++++++++++++------
> 1 file changed, 71 insertions(+), 22 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> index 11c3677..8638f43 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> @@ -34,32 +34,84 @@
> #define MAX_MOVS 8 /**< The maximum number of MOVs to attempt to match. */
>
> /**
> + * For a given control flow graph <cfg> and number <start_block> of the
> basic
> + * block ending with an ENDIF instruction, return pointers to the
> associated
> + * IF and ELSE instructions.
> + *
> + * If no ELSE instruction is found before the associated IF, return false.
> + * Otherwise return true.
> + */
> +static bool
> +find_if_else_from_endif(const cfg_t *cfg, int start_block,
> + fs_inst **if_inst, fs_inst **else_inst)
>
Two concerns about this function:
1. I'm worried that it's going to be fragile because it relies on an
undocumented behaviour of brw_cfg.cpp, namely the fact that the basic
blocks are generated in the order in which they appear in the program.
2. It seems like a lot of work to go to, given the fact that this
information is trivially available while we're building the cfg.
How about if instead we modify brw_cfg.cpp so that when it's building the
cfg, it records pointers from the endif block to the corresponding if and
else instructions? Then we could drop this function entirely and just pull
the information out of the cfg when we need it.
> +{
> + assert(start_block > 0);
> + assert(if_inst);
> + assert(else_inst);
> +
> + *if_inst = NULL;
> + *else_inst = NULL;
> +
> + int depth = 0;
> + for (int b = start_block - 1; b >= 0; b--) {
> + bblock_t *block = cfg->blocks[b];
> +
> + switch (block->end->opcode) {
> + case BRW_OPCODE_IF:
> + if (depth == 0) {
> + *if_inst = (fs_inst *) block->end;
> + return *else_inst != NULL;
> + }
> + depth--;
> + break;
> + case BRW_OPCODE_ELSE:
> + if (depth == 0) {
> + *else_inst = (fs_inst *) block->end;
> + }
> + /* No change in depth */
> + break;
> + case BRW_OPCODE_ENDIF:
> + case BRW_OPCODE_WHILE:
> + depth++;
> + break;
> + case BRW_OPCODE_DO:
> + depth--;
> + break;
> + default:
> + break;
> + }
> + }
> +
> + return false;
> +}
> +
> +/**
> * Scans backwards from an ENDIF counting MOV instructions with common
> * destinations inside the "then" and "else" blocks of the if statement.
> *
> - * A pointer to the fs_inst* for ENDIF is passed as the <match> argument.
> The
> - * function stores pointers to the MOV instructions in the <then_mov> and
> - * <else_mov> arrays. If the function is successful, the <match> points
> to the
> - * fs_inst* pointing to the IF instruction at the beginning of the block.
> + * A pointer to the fs_inst* for ENDIF is passed as the <endif_inst>
> argument.
> + * The function stores pointers to the MOV instructions in the <then_mov>
> and
> + * <else_mov> arrays.
> *
> * \return the number of MOVs to a common destination found in the two
> branches
> - * or zero if an error occurred.
> *
> * E.g.:
> - * match = IF ...
> + * IF ...
> + * then_mov[2] = MOV g2, ...
> * then_mov[1] = MOV g4, ...
> * then_mov[0] = MOV g5, ...
> * ELSE ...
> + * then_mov[2] = MOV g3, ...
> * else_mov[1] = MOV g4, ...
> * else_mov[0] = MOV g5, ...
> * ENDIF
> - * returns 2.
> + * returns 2 (since only the first two MOVs have a common destination)
>
Actually, for this example code it will return 3, since it's the caller
that checks whether the MOVs have a common destination.
Also, I failed to notice on my first read of this patch that it extends
match_movs_from_endif() to be able to handle non-MOV instructions inside
the IF/ELSE block. You might want to change your example to:
* IF ...
* ...other instructions...
* then_mov[2] = MOV g2, ...
* then_mov[1] = MOV g4, ...
* then_mov[0] = MOV g5, ...
* ELSE ...
* ...other instructions...
* then_mov[2] = MOV g3, ...
* else_mov[1] = MOV g4, ...
* else_mov[0] = MOV g5, ...
* ENDIF
just so that there's no confusion about that.
> */
> static int
> match_movs_from_endif(fs_inst *then_mov[MAX_MOVS], fs_inst
> *else_mov[MAX_MOVS],
> - fs_inst **match)
> + fs_inst *endif_inst, fs_inst *else_inst)
> {
> - fs_inst *m = *match;
> + fs_inst *m = endif_inst;
>
> assert(m->opcode == BRW_OPCODE_ENDIF);
> m = (fs_inst *) m->prev;
> @@ -71,9 +123,7 @@ match_movs_from_endif(fs_inst *then_mov[MAX_MOVS],
> fs_inst *else_mov[MAX_MOVS],
> else_movs++;
> }
>
> - if (m->opcode != BRW_OPCODE_ELSE)
> - return 0;
> - m = (fs_inst *) m->prev;
> + m = (fs_inst *) else_inst->prev;
>
> int then_movs = 0;
> while (then_movs < MAX_MOVS && m->opcode == BRW_OPCODE_MOV) {
> @@ -82,10 +132,6 @@ match_movs_from_endif(fs_inst *then_mov[MAX_MOVS],
> fs_inst *else_mov[MAX_MOVS],
> then_movs++;
> }
>
> - if (m->opcode != BRW_OPCODE_IF)
> - return 0;
> -
> - *match = m;
> return MIN2(then_movs, else_movs);
> }
>
> @@ -134,7 +180,7 @@ fs_visitor::opt_peephole_sel()
> bblock_t *block = cfg.blocks[b];
>
> int movs;
> - fs_inst *if_inst, *endif_inst;
> + fs_inst *if_inst, *else_inst, *endif_inst;
> fs_inst *start;
> fs_inst *else_mov[MAX_MOVS] = { NULL };
> fs_inst *then_mov[MAX_MOVS] = { NULL };
> @@ -145,14 +191,17 @@ fs_visitor::opt_peephole_sel()
> */
> start = (fs_inst *) block->end;
> if (start->opcode == BRW_OPCODE_ENDIF) {
> - fs_inst *match = endif_inst = start;
> + endif_inst = start;
> +
> + /* Find the associated IF and ELSE instructions for our ENDIF. */
> + if (!find_if_else_from_endif(&cfg, b, &if_inst, &else_inst))
> + continue;
>
> /* Find MOVs to a common destination. */
> - movs = match_movs_from_endif(then_mov, else_mov, &match);
> + movs = match_movs_from_endif(then_mov, else_mov, start,
> else_inst);
> +
> if (movs == 0)
> continue;
> -
> - if_inst = match;
> } else {
> continue;
> }
> @@ -171,7 +220,7 @@ fs_visitor::opt_peephole_sel()
> if (!then_mov[i]->dst.equals(else_mov[i]->dst) ||
> then_mov[i]->is_partial_write() ||
> else_mov[i]->is_partial_write()) {
> - bb_progress = false;
> + movs = i;
>
break;
> }
>
> --
> 1.8.3.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131030/8cec3f33/attachment-0001.html>
More information about the mesa-dev
mailing list