<div dir="ltr">On 28 October 2013 11:31, Matt Turner <span dir="ltr"><<a href="mailto:mattst88@gmail.com" target="_blank">mattst88@gmail.com</a>></span> wrote:<br><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">
fs_visitor::try_replace_with_sel optimizes only if statements whose<br>
"then" and "else" bodies contain a single MOV instruction. It also did<br>
could not handle constant arguments, since they cause an extra MOV<br></blockquote><div><br></div><div>s/did could not/could not/<br></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">
immediate to be generated (since we haven't run constant propagation,<br>
there are more than the single MOV).<br>
<br>
This peephole fixes both of these and operates as a normal optimization<br>
pass.<br>
<br>
fs_visitor::try_replace_with_sel is still arguably necessary, since it<br>
runs before pull constant loads are lowered.<br>
<br>
total instructions in shared programs: 1547180 -> 1545254 (-0.12%)<br>
instructions in affected programs: 96585 -> 94659 (-1.99%)<br>
---<br>
src/mesa/drivers/dri/i965/Makefile.sources | 1 +<br>
src/mesa/drivers/dri/i965/brw_fs.cpp | 1 +<br>
src/mesa/drivers/dri/i965/brw_fs.h | 1 +<br>
src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp | 245 ++++++++++++++++++++++<br>
4 files changed, 248 insertions(+)<br>
create mode 100644 src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources<br>
index c4d689e..5ddb421 100644<br>
--- a/src/mesa/drivers/dri/i965/Makefile.sources<br>
+++ b/src/mesa/drivers/dri/i965/Makefile.sources<br>
@@ -59,6 +59,7 @@ i965_FILES = \<br>
brw_fs_fp.cpp \<br>
brw_fs_generator.cpp \<br>
brw_fs_live_variables.cpp \<br>
+ brw_fs_sel_peephole.cpp \<br>
brw_fs_reg_allocate.cpp \<br>
brw_fs_vector_splitting.cpp \<br>
brw_fs_visitor.cpp \<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
index 28d369a..d3d2e44 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs.cpp<br>
@@ -3131,6 +3131,7 @@ fs_visitor::run()<br>
progress = opt_algebraic() || progress;<br>
progress = opt_cse() || progress;<br>
progress = opt_copy_propagate() || progress;<br>
+ progress = opt_peephole_sel() || progress;<br>
progress = dead_code_eliminate() || progress;<br>
progress = dead_code_eliminate_local() || progress;<br>
progress = register_coalesce() || progress;<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h<br>
index dff6ec1..a67ef86 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_fs.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs.h<br>
@@ -361,6 +361,7 @@ public:<br>
bool try_emit_saturate(ir_expression *ir);<br>
bool try_emit_mad(ir_expression *ir, int mul_arg);<br>
void try_replace_with_sel();<br>
+ bool opt_peephole_sel();<br>
void emit_bool_to_cond_code(ir_rvalue *condition);<br>
void emit_if_gen6(ir_if *ir);<br>
void emit_unspill(fs_inst *inst, fs_reg reg, uint32_t spill_offset,<br>
diff --git a/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp<br>
new file mode 100644<br>
index 0000000..11c3677<br>
--- /dev/null<br>
+++ b/src/mesa/drivers/dri/i965/brw_fs_sel_peephole.cpp<br>
@@ -0,0 +1,245 @@<br>
+/*<br>
+ * Copyright © 2013 Intel Corporation<br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS<br>
+ * IN THE SOFTWARE.<br>
+ */<br>
+<br>
+#include "brw_fs.h"<br>
+#include "brw_cfg.h"<br>
+<br>
+/** @file brw_fs_sel_peephole.cpp<br>
+ *<br>
+ * This file contains the opt_peephole_sel() optimization pass that replaces<br>
+ * MOV instructions to the same destination in the "then" and "else" bodies of<br>
+ * an if statement with SEL instructions.<br>
+ */<br>
+<br>
+#define MAX_MOVS 8 /**< The maximum number of MOVs to attempt to match. */<br></blockquote><div><br></div><div>Why 8 and not 4? Just general caution? Or have you found shaders that require 8?<br></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">
+<br>
+/**<br>
+ * Scans backwards from an ENDIF counting MOV instructions with common<br>
+ * destinations inside the "then" and "else" blocks of the if statement.<br>
+ *<br>
+ * A pointer to the fs_inst* for ENDIF is passed as the <match> argument. The<br>
+ * function stores pointers to the MOV instructions in the <then_mov> and<br>
+ * <else_mov> arrays. If the function is successful, the <match> points to the<br>
+ * fs_inst* pointing to the IF instruction at the beginning of the block.<br>
+ *<br>
+ * \return the number of MOVs to a common destination found in the two branches<br>
+ * or zero if an error occurred.<br></blockquote><div><br></div><div>This comment makes it sound like the function verifies that the set of destinations in the "then" and "else" blocks is the same. It doesn't--that's done by fs_visitor::opt_peephole_sel().<br>
</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">
+ *<br>
+ * E.g.:<br>
+ * match = IF ...<br>
+ * then_mov[1] = MOV g4, ...<br>
+ * then_mov[0] = MOV g5, ...<br>
+ * ELSE ...<br>
+ * else_mov[1] = MOV g4, ...<br>
+ * else_mov[0] = MOV g5, ...<br>
+ * ENDIF<br>
+ * returns 2.<br>
+ */<br>
+static int<br>
+match_movs_from_endif(fs_inst *then_mov[MAX_MOVS], fs_inst *else_mov[MAX_MOVS],<br>
+ fs_inst **match)<br>
+{<br>
+ fs_inst *m = *match;<br>
+<br>
+ assert(m->opcode == BRW_OPCODE_ENDIF);<br>
+ m = (fs_inst *) m->prev;<br>
+<br>
+ int else_movs = 0;<br>
+ while (else_movs < MAX_MOVS && m->opcode == BRW_OPCODE_MOV) {<br>
+ else_mov[else_movs] = m;<br>
+ m = (fs_inst *) m->prev;<br>
+ else_movs++;<br>
+ }<br>
+<br>
+ if (m->opcode != BRW_OPCODE_ELSE)<br>
+ return 0;<br>
+ m = (fs_inst *) m->prev;<br>
+<br>
+ int then_movs = 0;<br>
+ while (then_movs < MAX_MOVS && m->opcode == BRW_OPCODE_MOV) {<br>
+ then_mov[then_movs] = m;<br>
+ m = (fs_inst *) m->prev;<br>
+ then_movs++;<br>
+ }<br>
+<br>
+ if (m->opcode != BRW_OPCODE_IF)<br>
+ return 0;<br>
+<br>
+ *match = m;<br>
+ return MIN2(then_movs, else_movs);<br>
+}<br>
+<br>
+/**<br>
+ * Try to replace IF/MOV+/ELSE/MOV+/ENDIF with SEL.<br>
+ *<br>
+ * Many GLSL shaders contain the following pattern:<br>
+ *<br>
+ * x = condition ? foo : bar<br>
+ *<br>
+ * or<br>
+ *<br>
+ * if (...) a.xyzw = foo.xyzw;<br>
+ * else a.xyzw = bar.xyzw;<br>
+ *<br>
+ * The compiler emits an ir_if tree for this, since each subexpression might be<br>
+ * a complex tree that could have side-effects or short-circuit logic.<br>
+ *<br>
+ * However, the common case is to simply select one of two constants or<br>
+ * variable values---which is exactly what SEL is for. In this case, the<br>
+ * assembly looks like:<br>
+ *<br>
+ * (+f0) IF<br>
+ * ...<br>
+ * MOV dst src0<br>
+ * ELSE<br>
+ * ...<br>
+ * MOV dst src1<br>
+ * ENDIF<br>
+ *<br>
+ * where each pair of MOVs to a common destination and can be easily translated<br>
+ * into<br>
+ *<br>
+ * (+f0) SEL dst src0 src1<br>
+ *<br>
+ * If src0 is an immediate value, we promote it to a temporary GRF.<br>
+ */<br>
+bool<br>
+fs_visitor::opt_peephole_sel()<br>
+{<br>
+ bool progress = false;<br>
+<br>
+ cfg_t cfg(this);<br>
+<br>
+ for (int b = 0; b < cfg.num_blocks; b++) {<br>
+ bblock_t *block = cfg.blocks[b];<br>
+<br>
+ int movs;<br>
+ fs_inst *if_inst, *endif_inst;<br>
+ fs_inst *start;<br>
+ fs_inst *else_mov[MAX_MOVS] = { NULL };<br>
+ fs_inst *then_mov[MAX_MOVS] = { NULL };<br>
+ bool bb_progress = false;<br>
+<br>
+ /* IF and ENDIF instructions, by definition, can only be found at the<br>
+ * ends of basic blocks.<br>
+ */<br>
+ start = (fs_inst *) block->end;<br>
+ if (start->opcode == BRW_OPCODE_ENDIF) {<br>
+ fs_inst *match = endif_inst = start;<br>
+<br>
+ /* Find MOVs to a common destination. */<br>
+ movs = match_movs_from_endif(then_mov, else_mov, &match);<br>
+ if (movs == 0)<br>
+ continue;<br>
+<br>
+ if_inst = match;<br>
+ } else {<br>
+ continue;<br>
+ }<br>
+<br>
+ assert(if_inst && endif_inst);<br>
+<br>
+ fs_inst *sel_inst[MAX_MOVS] = { NULL };<br>
+ fs_inst *mov_imm_inst[MAX_MOVS] = { NULL };<br>
+<br>
+ /* Generate SEL instructions for pairs of MOVs to a common destination. */<br>
+ for (int i = 0; i < movs; i++) {<br>
+ if (!then_mov[i] || !else_mov[i])<br>
+ break;<br>
+<br>
+ /* Check that the MOVs are the right form. */<br>
+ if (!then_mov[i]->dst.equals(else_mov[i]->dst) ||<br>
+ then_mov[i]->is_partial_write() ||<br>
+ else_mov[i]->is_partial_write()) {<br>
+ bb_progress = false;<br></blockquote><div><br></div><div>I found bb_progress difficult to follow, because we set it to true at the bottom of this loop (before we've definitively made progress) and then reset it to false here if there's a malformed MOV. How about if instead we make a boolean called "malformed_mov_found", which starts off false and gets set to true here.<br>
</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">
+ break;<br>
+ }<br>
+<br>
+ /* Only the last source register can be a constant, so if the MOV in<br>
+ * the "then" clause uses a constant, we need to put it in a<br>
+ * temporary.<br>
+ */<br>
+ fs_reg src0(then_mov[i]->src[0]);<br>
+ if (src0.file == IMM) {<br>
+ src0 = fs_reg(this, glsl_type::float_type);<br>
+ src0.type = then_mov[i]->src[0].type;<br>
+ mov_imm_inst[i] = MOV(src0, then_mov[i]->src[0]);<br>
+ }<br>
+<br>
+ sel_inst[i] = SEL(then_mov[i]->dst, src0, else_mov[i]->src[0]);<br>
+<br>
+ if (brw->gen == 6 && if_inst->conditional_mod) {<br>
+ /* For Sandybridge with IF with embedded comparison */<br>
+ sel_inst[i]->predicate = BRW_PREDICATE_NORMAL;<br>
+ } else {<br>
+ /* Separate CMP and IF instructions */<br>
+ sel_inst[i]->predicate = if_inst->predicate;<br>
+ sel_inst[i]->predicate_inverse = if_inst->predicate_inverse;<br>
+ }<br>
+<br>
+ bb_progress = true;<br>
+ }<br>
+<br>
+ if (bb_progress) {<br></blockquote><div><br></div><div>Then this would simply be "if (!malformed_mov_found)"<br></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">
+ fs_inst *cmp_inst;<br>
+ if (brw->gen == 6 && if_inst->conditional_mod) {<br>
+ cmp_inst = CMP(reg_null_d, if_inst->src[0], if_inst->src[1],<br>
+ if_inst->conditional_mod);<br>
+ } else {<br>
+ /* Separate CMP and IF instructions. Find instruction that wrote<br>
+ * the flag register.<br>
+ */<br>
+ fs_inst *m;<br>
+ for (m = (fs_inst *) if_inst->prev; !m->writes_flag();<br>
+ m = (fs_inst *) m->prev);<br>
+<br>
+ cmp_inst = new(mem_ctx) fs_inst(m);<br>
+ }<br></blockquote><div><br></div><div>This seems like a problem because in the non-gen6 case, we'll search back arbitrarily far to find the instruction that wrote to the flag register; it's not necessarily safe to copy that instruction to after the ENDIF block. For example, if the input code is:<br>
<br></div><div>CMP null, a, b<br></div><div>MOV a, c<br></div><div>IF<br></div><div> MOV d, e<br></div><div>ELSE<br></div><div> MOV d, f<br></div><div>ENDIF<br><br></div><div>Then we'll incorrectly optimize it to:<br>
<br></div><div>CMP null, a, b # will later be dead code eliminated<br></div><div>MOV a, c<br></div>IF # will later be dead code eliminated<br><div>ELSE # will later be dead code eliminated</div><div>ENDIF # will later be dead code eliminated</div>
<div><div>CMP null, a, b # incorrect! a has changed.<br></div>SEL d, e, f<br><br></div><div>I think what we want to do instead is emit the SEL instructions before the IF; that way the condition flag will still be valid, so we don't have to make a copy of the CMP instruction and we can just optimize to:<br>
<br></div><div>CMP null, a, b<br></div><div>MOV a, c<br></div><div>SEL d, e, f<br>IF # will later be dead code eliminated<br><div>ELSE # will later be dead code eliminated</div><div>ENDIF # will later be dead code eliminated</div>
</div><div><br></div><div>A side bonus of this approach is that we produce fewer instructions that dead code elimination has to reclaim.<br></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">
+<br>
+ /* Insert flag-writing instruction immediately after the ENDIF, and<br>
+ * SEL and MOV imm instructions after that.<br>
+ */<br>
+ if (start->opcode == BRW_OPCODE_ENDIF) {<br>
+ endif_inst->insert_after(cmp_inst);<br>
+<br>
+ for (int i = 0; i < movs; i++) {<br>
+ cmp_inst->insert_after(sel_inst[i]);<br>
+ if (mov_imm_inst[i])<br>
+ cmp_inst->insert_after(mov_imm_inst[i]);<br>
+<br>
+ then_mov[i]->remove();<br>
+ else_mov[i]->remove();<br>
+ }<br>
+<br>
+ /* Appending an instruction may have changed our bblock end. */<br>
+ block->end = sel_inst[0];<br>
+ } <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+ progress = progress || bb_progress;<br></blockquote><div><br></div><div>I think it would read slightly better to simply do<br><br></div><div> progress = true;<br><br>inside the "if (bb_progress)" block (or inside the "if (!malformed_mov_found)" block, if you take my suggestion about malformed_mov_found above). But it's not a big deal.<br>
<br>My only critical concern is about the non-gen6 code path leading to incorrect optimizations.<br></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">
+ }<br>
+<br>
+ if (progress)<br>
+ invalidate_live_intervals();<br>
+<br>
+ return progress;<br>
+}<br>
<span class=""><font color="#888888">--<br>
1.8.3.2<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>