<div dir="ltr">On 29 October 2013 23:37, 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:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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) </blockquote><div> </div><div>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. </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +{ + 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) </blockquote><div> </div><div>Actually, for this example code it will return 3, since it's the caller that checks whether the MOVs have a common destination. </div><div>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 </div><div>just so that there's no confusion about that. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> */ 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; </blockquote> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> break; } -- 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>