<div dir="ltr">On 28 October 2013 11:31, Matt Turner <<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 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) </blockquote><div> </div><div>I have similar concerns about this function to the concerns I had about find_if_else_from_endif() in patch 10.5. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> +{ + 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; </blockquote><div> </div><div>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." </div><div>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." </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> } 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); </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + + 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 <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>