<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jul 23, 2018 at 3:03 AM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">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">Since we know what side of the branch we ended up on we can just<br>
replace the use with a constant.<br>
<br>
All the spill changes in shader-db are from Dolphin uber shaders,<br>
despite some small regressions the change is clearly positive.<br>
<br>
shader-db results IVB:<br>
<br>
total instructions in shared programs: 9999201 -> 9993483 (-0.06%)<br>
instructions in affected programs: 163235 -> 157517 (-3.50%)<br>
helped: 132<br>
HURT: 2<br>
<br>
total cycles in shared programs: 231670754 -> 219476091 (-5.26%)<br>
cycles in affected programs: 143424120 -> 131229457 (-8.50%)<br>
helped: 115<br>
HURT: 24<br>
<br>
total spills in shared programs: 4383 -> 4370 (-0.30%)<br>
spills in affected programs: 1656 -> 1643 (-0.79%)<br>
helped: 9<br>
HURT: 18<br>
<br>
total fills in shared programs: 4610 -> 4581 (-0.63%)<br>
fills in affected programs: 374 -> 345 (-7.75%)<br>
helped: 6<br>
HURT: 0<br>
---<br>
 src/compiler/nir/nir_opt_if.c | 124 ++++++++++++++++++++++++++++++++++<br>
 1 file changed, 124 insertions(+)<br>
<br>
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c<br>
index b3d0bf1decb..b3d5046a76e 100644<br>
--- a/src/compiler/nir/nir_opt_if.c<br>
+++ b/src/compiler/nir/nir_opt_if.c<br>
@@ -369,6 +369,87 @@ opt_if_loop_terminator(nir_if *nif)<br>
    return true;<br>
 }<br>
<br>
+static void<br>
+replace_if_condition_use_with_const(nir_src *use, unsigned nir_boolean,<br>
+                                    void *mem_ctx, bool if_condition)<br>
+{<br>
+   /* Create const */<br>
+   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32);<br>
+   load->value.u32[0] = nir_boolean;<br>
+<br>
+   if (if_condition) {<br>
+      nir_instr_insert_before_cf(&use->parent_if->cf_node,  &load->instr);<br></blockquote><div><br></div><div>If it was me, I'd probably use the builder but I think it's a wash in this case.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+   } else if (use->parent_instr->type == nir_instr_type_phi) {<br>
+      nir_phi_instr *cond_phi = nir_instr_as_phi(use->parent_instr);<br>
+<br>
+      bool UNUSED found = false;<br>
+      nir_foreach_phi_src(phi_src, cond_phi) {<br>
+         if (phi_src->src.ssa == use->ssa) {<br></blockquote><div><br></div><div>You could also just use some sort of container_of macro to cast from the src to a phi_src.  It's a bit sneaky so maybe not a good idea for the tiny bit of perf.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            nir_instr_insert_before_block(phi_src->pred, &load->instr);<br></blockquote><div><br></div><div>after_block_before_jump would work just as well and would put the load_const closer to its use.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+            found = true;<br>
+            break;<br>
+         }<br>
+      }<br>
+      assert(found);<br>
+   } else {<br>
+      nir_instr_insert_before(use->parent_instr,  &load->instr);<br>
+   }<br>
+<br>
+   /* Rewrite use to use const */<br>
+   nir_src new_src = nir_src_for_ssa(&load->def);<br></blockquote><div><br></div><div>Is there a good reason for the temporary variable?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+   if (if_condition)<br>
+      nir_if_rewrite_condition(use->parent_if, new_src);<br>
+   else<br>
+      nir_instr_rewrite_src(use->parent_instr, use, new_src);<br>
+} <br></blockquote><div><br></div><div>Ok, enough nitpicking.  None of the above things are actually problems.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+static bool<br>
+evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,<br>
+                       bool if_condition)<br>
+{<br>
+   bool progress = false;<br>
+<br>
+   nir_block *use_block;<br>
+   if (if_condition) {<br>
+      use_block =<br>
+         nir_cf_node_as_block(nir_cf_node_prev(&use_src->parent_if->cf_node));<br>
+   } else {<br>
+      use_block = use_src->parent_instr->block;<br></blockquote><div><br></div><div>Not true for phis!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+   }<br>
+<br>
+   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {<br>
+      replace_if_condition_use_with_const(use_src, NIR_TRUE, mem_ctx,<br>
+                                          if_condition);<br>
+      progress = true;<br>
+   } else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)) {<br>
+      replace_if_condition_use_with_const(use_src, NIR_FALSE, mem_ctx,<br>
+                                          if_condition);<br>
+      progress = true;<br>
+   }<br>
+<br>
+   return progress;<br>
+}<br></blockquote><div><br></div><div>I think things would be more straightforward (and correct!) if you merged the above two functions and restructured them a bit as follows:</div><div><br></div><div>static bool</div><div>try_rewrite_if_use(nir_builder *b, nir_if *nif, nir_src *src, bool if_condition)</div><div>{</div><div>   if (if_condition) {</div><div>      b->cursor = nir_before_cf_node(&nif->cf_node);<br></div><div>   } else if (src->parent_instr->type == nir_instr_type_phi) {</div><div>      // Set the cursor and use_block to the predecessor block<br></div><div>   } else {</div><div>      b->cursor = nir_before_instr(src->parent_instr);<br></div><div>   }</div><div>   nir_block *use_block = nir_cursor_current_block(b->cursor);<br></div><div><br></div><div>   nir_ssa_def *const_val;<br></div><div>   if (nir_block_dominates(nir_if_first_then_block(nif), use_block))</div><div>      const_value = nir_imm_int(b, NIR_TRUE);</div><div>   else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)</div><div>      const_value = nir_imm_int(b, NIR_FALSE);</div><div>   else</div><div>      return false;</div><div><br></div><div>   // Rewrite the source</div><div><br></div><div>   return true;<br></div><div>}</div><div><br></div><div>Other than the phi issue I pointed out above (which would be fixed by that refactor), everything looks good to me.</div><div><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+static bool<br>
+opt_if_evaluate_condition_use(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>
+   }<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>
+   }<br>
+<br>
+   return progress;<br>
+}<br>
+<br>
 static bool<br>
 opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)<br>
 {<br>
@@ -402,6 +483,41 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)<br>
    return progress;<br>
 }<br>
<br>
+/**<br>
+ * These optimisations depend on nir_metadata_block_index and therefore must<br>
+ * not do anything to cause the metadata to become invalid.<br>
+ */<br>
+static bool<br>
+opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list, void *mem_ctx)<br>
+{<br>
+   bool progress = false;<br>
+   foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {<br>
+      switch (cf_node->type) {<br>
+      case nir_cf_node_block:<br>
+         break;<br>
+<br>
+      case nir_cf_node_if: {<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>
+         break;<br>
+      }<br>
+<br>
+      case nir_cf_node_loop: {<br>
+         nir_loop *loop = nir_cf_node_as_loop(cf_node);<br>
+         progress |= opt_if_safe_cf_list(b, &loop->body, mem_ctx);<br>
+         break;<br>
+      }<br>
+<br>
+      case nir_cf_node_function:<br>
+         unreachable("Invalid cf type");<br>
+      }<br>
+   }<br>
+<br>
+   return progress;<br>
+}<br>
+<br>
 bool<br>
 nir_opt_if(nir_shader *shader)<br>
 {<br>
@@ -414,6 +530,14 @@ nir_opt_if(nir_shader *shader)<br>
       nir_builder b;<br>
       nir_builder_init(&b, function->impl);<br>
<br>
+      void *mem_ctx = ralloc_parent(function->impl);<br>
+<br>
+      nir_metadata_require(function->impl, nir_metadata_block_index |<br>
+                           nir_metadata_dominance);<br>
+      progress = opt_if_safe_cf_list(&b, &function->impl->body, mem_ctx);<br>
+      nir_metadata_preserve(function->impl, nir_metadata_block_index |<br>
+                            nir_metadata_dominance);<br>
+<br>
       if (opt_if_cf_list(&b, &function->impl->body)) {<br>
          nir_metadata_preserve(function->impl, nir_metadata_none);<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>