<div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 27, 2018 at 4:09 AM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com" target="_blank">tarceri@itsqueeze.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">v2:<br>
 - only allow nir_op_inot or nir_op_b2i when alu input is 1.<br>
 - use some helpers as suggested by Jason.<br>
<br>
shader-db IVB results:<br>
<br>
total instructions in shared programs: 9993483 -> 9993472 (-0.00%)<br>
instructions in affected programs: 1300 -> 1289 (-0.85%)<br>
helped: 11<br>
HURT: 0<br>
<br>
total cycles in shared programs: 219476091 -> 219476059 (-0.00%)<br>
cycles in affected programs: 7675 -> 7643 (-0.42%)<br>
helped: 10<br>
HURT: 1<br>
---<br>
 src/compiler/nir/nir_opt_if.c | 135 ++++++++++++++++++++++++++++++++--<br>
 1 file changed, 129 insertions(+), 6 deletions(-)<br>
<br>
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c<br>
index 11c6693d302..6d81705f6b7 100644<br>
--- a/src/compiler/nir/nir_opt_if.c<br>
+++ b/src/compiler/nir/nir_opt_if.c<br>
@@ -403,9 +403,113 @@ evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)<br>
    }<br>
 }<br>
<br>
+/*<br>
+ * This propagates if condition evaluation down the chain of some alu<br>
+ * instructions. For example by checking the use of some of the following alu<br>
+ * instruction we can eventually replace ssa_107 with NIR_TRUE.<br>
+ *<br>
+ *   loop {<br>
+ *      block block_1:<br>
+ *      vec1 32 ssa_85 = load_const (0x00000002)<br>
+ *      vec1 32 ssa_86 = ieq ssa_48, ssa_85<br>
+ *      vec1 32 ssa_87 = load_const (0x00000001)<br>
+ *      vec1 32 ssa_88 = ieq ssa_48, ssa_87<br>
+ *      vec1 32 ssa_89 = ior ssa_86, ssa_88<br>
+ *      vec1 32 ssa_90 = ieq ssa_48, ssa_0<br>
+ *      vec1 32 ssa_91 = ior ssa_89, ssa_90<br>
+ *      if ssa_86 {<br>
+ *         block block_2:<br>
+ *             ...<br>
+ *            break<br>
+ *      } else {<br>
+ *            block block_3:<br>
+ *      }<br>
+ *      block block_4:<br>
+ *      if ssa_88 {<br>
+ *            block block_5:<br>
+ *             ...<br>
+ *            break<br>
+ *      } else {<br>
+ *            block block_6:<br>
+ *      }<br>
+ *      block block_7:<br>
+ *      if ssa_90 {<br>
+ *            block block_8:<br>
+ *             ...<br>
+ *            break<br>
+ *      } else {<br>
+ *            block block_9:<br>
+ *      }<br>
+ *      block block_10:<br>
+ *      vec1 32 ssa_107 = inot ssa_91<br>
+ *      if ssa_107 {<br>
+ *            block block_11:<br>
+ *            break<br>
+ *      } else {<br>
+ *            block block_12:<br>
+ *      }<br>
+ *   }<br>
+ */<br>
 static bool<br>
-evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,<br>
-                       bool is_if_condition)<br>
+propagate_condition_eval(nir_builder *b, nir_if *nif, nir_src *use_src,<br>
+                         nir_src *alu_use, nir_alu_instr *alu, void *mem_ctx,<br>
+                         bool is_if_condition)<br>
+{<br>
+   bool progress = false;<br>
+<br>
+   uint32_t bool_value;<br>
+   nir_cursor cursor = nir_before_src(alu_use, is_if_condition);<br>
+   if (nir_op_infos[alu->op].num_inputs == 1) {<br></blockquote><div><br></div><div>Howa about just</div><div><br></div><div>if (alu->op == nir_op_inot || alu->op == nir_op_b2i) {</div><div>   if (evaluate_if_condition()) {</div><div>      replace_if_condition_with_const();</div><div>      return true;<br></div><div>   }<br></div><div>} else if (alu->op == alu_op_ior || alu_op_iand) {<br><div>   if (evaluate_if_condition()) {</div><div>      // stuff</div><div>      return true;<br></div><div>   }</div><div>}</div><div><br></div><div>return false;<br></div></div><div><br></div><div>Or, for that matter, it could be a switch statement.  My point is that checking the number of sources and then checking the number of inputs seems kind-of pointless.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+      if ((alu->op == nir_op_inot || alu->op == nir_op_b2i) &&<br>
+          evaluate_if_condition(nif, cursor, &bool_value)) {<br>
+         replace_if_condition_use_with_const(alu_use, mem_ctx, cursor,<br>
+                                             bool_value, is_if_condition);<br></blockquote><div><br></div><div>This isn't correct.  We need to run nir_eval_const_opcode on it before replacing the ALU destination with it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+         progress = true;<br>
+      }<br>
+   } else {<br>
+      assert(alu->op == nir_op_ior || alu->op == nir_op_iand);<br>
+<br>
+      if (evaluate_if_condition(nif, cursor, &bool_value)) {<br>
+         nir_ssa_def *def[2];<br>
+         for (unsigned i = 0; i < 2; i++) {<br>
+            if (alu->src[i].src.ssa == use_src->ssa) {<br>
+               if (is_if_condition) {<br>
+                  b->cursor =<br>
+                     nir_before_cf_node(&alu_use->parent_if->cf_node);<br>
+               } else {<br>
+                  b->cursor = nir_before_instr(alu_use->parent_instr);<br>
+               }<br></blockquote><div><br></div><div>This should be nir_before_src<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+               nir_const_value const_value;<br>
+               const_value.u32[0] = bool_value;<br>
+<br>
+               def[i] = nir_build_imm(b, 1, 32, const_value);<br>
+            } else {<br>
+               def[i] = alu->src[i].src.ssa;<br>
+            }<br>
+         }<br>
+<br>
+         nir_ssa_def *nalu =<br>
+            nir_build_alu(b, alu->op, def[0], def[1], NULL, NULL);<br>
+<br>
+         /* Rewrite use to use new alu instruction */<br>
+         nir_src new_src = nir_src_for_ssa(nalu);<br>
+<br>
+         if (is_if_condition)<br>
+            nir_if_rewrite_condition(alu_use->parent_if, new_src);<br>
+         else<br>
+            nir_instr_rewrite_src(alu_use->parent_instr, alu_use, new_src);<br>
+<br>
+         progress = true;<br>
+      }<br>
+   }<br>
+<br>
+   return progress;<br>
+}<br>
+<br>
+static bool<br>
+evaluate_condition_use(nir_builder *b, nir_if *nif, nir_src *use_src,<br>
+                       void *mem_ctx, bool is_if_condition)<br>
 {<br>
    bool progress = false;<br>
<br>
@@ -417,23 +521,42 @@ evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,<br>
       progress = true;<br>
    }<br>
<br>
+   if (!is_if_condition &&<br>
+       use_src->parent_instr->type == nir_instr_type_alu &&<br>
+       (nir_instr_as_alu(use_src->parent_instr)->op == nir_op_ior ||<br>
+        nir_instr_as_alu(use_src->parent_instr)->op == nir_op_iand ||<br>
+        nir_op_infos[nir_instr_as_alu(use_src->parent_instr)->op].num_inputs == 1)) {<br></blockquote><div><br></div><div>So we're checking for exact opcodes for binops  here but leaving the check for unops to the function we call?  Let's pick one.  How about having a little can_propagate_through_alu helper and call that here?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+         nir_alu_instr *alu = nir_instr_as_alu(use_src->parent_instr);<br>
+<br>
+         nir_foreach_use_safe(alu_use, &alu->dest.dest.ssa) {<br>
+            progress |= propagate_condition_eval(b, nif, use_src, alu_use, alu,<br>
+                                                 mem_ctx, false);<br>
+         }<br>
+<br>
+         nir_foreach_if_use_safe(alu_use, &alu->dest.dest.ssa) {<br>
+            progress |= propagate_condition_eval(b, nif, use_src, alu_use, alu,<br>
+                                                 mem_ctx, true);<br>
+         }<br>
+   }<br>
+<br>
    return progress;<br>
 }<br>
<br>
 static bool<br>
-opt_if_evaluate_condition_use(nir_if *nif, void *mem_ctx)<br>
+opt_if_evaluate_condition_use(nir_builder *b, nir_if *nif, void *mem_ctx)<br>
 {<br>
    bool progress = false;<br>
<br>
    /* Evaluate any uses of the if condition inside the if branches */<br>
    assert(nif->condition.is_ssa);<br>
    nir_foreach_use_safe(use_src, nif->condition.ssa) {<br>
-      progress |= evaluate_condition_use(nif, use_src, mem_ctx, false);<br>
+      progress |= evaluate_condition_use(b, nif, use_src, mem_ctx, false);<br>
    }<br>
<br>
    nir_foreach_if_use_safe(use_src, nif->condition.ssa) {<br>
       if (use_src->parent_if != nif)<br>
-         progress |= evaluate_condition_use(nif, use_src, mem_ctx, true);<br>
+         progress |= evaluate_condition_use(b, nif, use_src, mem_ctx, true);<br>
    }<br>
<br>
    return progress;<br>
@@ -489,7 +612,7 @@ opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list, void *mem_ctx)<br>
          nir_if *nif = nir_cf_node_as_if(cf_node);<br>
          progress |= opt_if_safe_cf_list(b, &nif->then_list, mem_ctx);<br>
          progress |= opt_if_safe_cf_list(b, &nif->else_list, mem_ctx);<br>
-         progress |= opt_if_evaluate_condition_use(nif, mem_ctx);<br>
+         progress |= opt_if_evaluate_condition_use(b, nif, mem_ctx);<br>
          break;<br>
       }<br>
<br>
-- <br>
2.17.1<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote></div></div></div>