[Mesa-dev] [PATCH 12/15] i965/fs: Extend SEL peephole to search from the IF down.
Paul Berry
stereotype441 at gmail.com
Wed Oct 30 18:46:39 CET 2013
On 28 October 2013 11:31, Matt Turner <mattst88 at gmail.com> wrote:
> The pass previously searched only backward from an ENDIF (and ELSE) to
> find MOVs with the same destination. This commit extends the pass to
> also search forward from the IF (and ELSE) to find matching MOVs which
> it can replace with SEL instructions before the IF.
>
> E.g., the pass can now optimize
>
> (+f0) IF
> MOV dst0 ...
> MOV dst1 ...
> MOV dst2 ...
> ELSE
> MOV dst0 ...
> MOV dst1 ...
> MOV dst3 ...
> ENDIF
>
> total instructions in shared programs: 1287457 -> 1287432 (-0.00%)
> instructions in affected programs: 15664 -> 15639 (-0.16%)
> ---
> src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp | 131
> +++++++++++++++++++++-
> 1 file changed, 129 insertions(+), 2 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 4c0eba9..ebde78b 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp
> @@ -86,6 +86,58 @@ find_if_else_from_endif(const cfg_t *cfg, int
> start_block,
> }
>
> /**
> + * For a given control flow graph <cfg> and number <start_block> of the
> basic
> + * block ending with an IF instruction, return pointers to the associated
> + * ELSE and ENDIF instructions.
> + *
> + * If no ELSE instruction is found before the associated ENDIF, return
> false.
> + * Otherwise return true.
> + */
> +static bool
> +find_else_endif_from_if(const cfg_t *cfg, int start_block,
> + fs_inst **else_inst, fs_inst **endif_inst)
>
I have similar concerns about this function to the concerns I had about
find_if_else_from_endif() in patch 10.5.
> +{
> + assert(start_block < cfg->num_blocks - 1);
> + assert(else_inst);
> + assert(endif_inst);
> +
> + *else_inst = NULL;
> + *endif_inst = NULL;
> +
> + int depth = 0;
> + for (int b = start_block + 1; b < cfg->num_blocks; b++) {
> + bblock_t *block = cfg->blocks[b];
> +
> + switch (block->end->opcode) {
> + case BRW_OPCODE_ENDIF:
> + if (depth == 0) {
> + *endif_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_IF:
> + case BRW_OPCODE_DO:
> + depth++;
> + break;
> + case BRW_OPCODE_WHILE:
> + 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.
> *
> @@ -136,6 +188,56 @@ match_movs_from_endif(fs_inst *then_mov[MAX_MOVS],
> fs_inst *else_mov[MAX_MOVS],
> }
>
> /**
> + * Scans forward from an IF counting MOV instructions with common
> destinations
> + * inside the "then" and "else" blocks of the if statement.
> + *
> + * A pointer to the fs_inst* for IF is passed as the <if_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
> + *
> + * E.g.:
> + * IF ...
> + * then_mov[0] = MOV g4, ...
> + * then_mov[1] = MOV g5, ...
> + * then_mov[2] = MOV g6, ...
> + * ELSE ...
> + * else_mov[0] = MOV g4, ...
> + * else_mov[1] = MOV g5, ...
> + * else_mov[2] = MOV g7, ...
> + * ENDIF
> + * returns 2 (since only the first two MOVs have a common destination)
> + */
> +static int
> +match_movs_from_if(fs_inst *then_mov[MAX_MOVS], fs_inst
> *else_mov[MAX_MOVS],
> + fs_inst *if_inst, fs_inst *else_inst)
> +{
> + fs_inst *m = if_inst;
> +
> + assert(m->opcode == BRW_OPCODE_IF);
> + m = (fs_inst *) m->next;
> +
> + int then_movs = 0;
> + while (then_movs < MAX_MOVS && m->opcode == BRW_OPCODE_MOV) {
> + then_mov[then_movs] = m;
> + m = (fs_inst *) m->next;
> + then_movs++;
> + }
> +
> + m = (fs_inst *) else_inst->next;
> +
> + int else_movs = 0;
> + while (else_movs < MAX_MOVS && m->opcode == BRW_OPCODE_MOV) {
> + else_mov[else_movs] = m;
> + m = (fs_inst *) m->next;
> + else_movs++;
> + }
> +
> + return MIN2(then_movs, else_movs);
> +}
> +
> +/**
> * Try to replace IF/MOV+/ELSE/MOV+/ENDIF with SEL.
> *
> * Many GLSL shaders contain the following pattern:
> @@ -202,6 +304,18 @@ fs_visitor::opt_peephole_sel()
>
> if (movs == 0)
> continue;
> + } else if (start->opcode == BRW_OPCODE_IF) {
> + if_inst = start;
> +
> + /* Find the associated ELSE and ENDIF instructions for our IF. */
> + if (!find_else_endif_from_if(&cfg, b, &else_inst, &endif_inst))
> + continue;
> +
> + /* Find MOVs to a common destination. */
> + movs = match_movs_from_if(then_mov, else_mov, start, else_inst);
> +
> + if (movs == 0)
> + continue;
>
It seems like we're doing some redundant work between this newly added code
and the block above it.
The block above it says "for each 'if', find the matching else and endif.
Then try to peephole SEL by walking forward through the then/else
instructions."
The block below it says "for each 'endif', find the matching if and else.
Then try to peephole SEL by walking backward through the then/else
instructions."
It seems like we could just as easily do: "For each 'endif', find the
matching if and else. Then try to peephole SEL by walking forward through
the then/else instructions. Then try to peephole SEL by walking backward
through the then/else instructions".
An advantage of this approach is that there's no ambiguity about whether we
walk forward first or walk backward first. And I think walking forward
first is better, because it avoids the problem we talked about in patch 9
of needing to save and restore the flag register.
> } else {
> continue;
> }
> @@ -269,8 +383,8 @@ fs_visitor::opt_peephole_sel()
> cmp_inst = new(mem_ctx) fs_inst(m);
> }
>
> - /* Insert flag-writing instruction immediately after the ENDIF,
> and
> - * SEL and MOV imm instructions after that.
> + /* Insert flag-writing instruction immediately after the IF or
> ENDIF,
> + * and SEL and MOV imm instructions after that.
> */
> if (start->opcode == BRW_OPCODE_ENDIF) {
> endif_inst->insert_after(cmp_inst);
> @@ -286,6 +400,19 @@ fs_visitor::opt_peephole_sel()
>
> /* Appending an instruction may have changed our bblock end.
> */
> block->end = sel_inst[0];
> + } else if (start->opcode == BRW_OPCODE_IF) {
> + if_inst->insert_before(cmp_inst);
>
With the exception of the gen6 if-with-cmod case, this line can be dropped,
and then we don't have the problem with the cmp instruction getting moved
to a place where it might have a different meaning.
> +
> + for (int i = 0; i < movs; i++) {
> + cmp_inst->insert_after(sel_inst[i]);
> + if (mov_imm_inst[i])
> + cmp_inst->insert_after(mov_imm_inst[i]);
> +
> + then_mov[i]->remove();
> + else_mov[i]->remove();
> + }
> +
> + /* No change to basic block boundaries possible. */
> }
> }
> progress = progress || bb_progress;
> --
> 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/b24bea1d/attachment.html>
More information about the mesa-dev
mailing list