On 30/08/18 10:57, Ian Romanick wrote:
On 08/27/2018 02:08 AM, Timothy Arceri wrote:
Since we know what side of the branch we ended up on we can just
replace the use with a constant.

All the spill changes in shader-db are from Dolphin uber shaders,
despite some small regressions the change is clearly positive.

V2: insert new constant after any phis in the
     use->parent_instr->type == nir_instr_type_phi path.

v3:
  - use nir_after_block_before_jump() for inserting const
  - check dominance of phi uses correctly

v4:
  - create some helpers as suggested by Jason.

shader-db results IVB:

total instructions in shared programs: 9999201 -> 9993483 (-0.06%)
instructions in affected programs: 163235 -> 157517 (-3.50%)
helped: 132
HURT: 2

total cycles in shared programs: 231670754 -> 219476091 (-5.26%)
cycles in affected programs: 143424120 -> 131229457 (-8.50%)
helped: 115
HURT: 24

total spills in shared programs: 4383 -> 4370 (-0.30%)
spills in affected programs: 1656 -> 1643 (-0.79%)
helped: 9
HURT: 18

total fills in shared programs: 4610 -> 4581 (-0.63%)
fills in affected programs: 374 -> 345 (-7.75%)
helped: 6
HURT: 0
---
  src/compiler/nir/nir.h        |  22 +++++++
  src/compiler/nir/nir_opt_if.c | 113 ++++++++++++++++++++++++++++++++++
  2 files changed, 135 insertions(+)

diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
index 009a6d60371..0caacd30173 100644
--- a/src/compiler/nir/nir.h
+++ b/src/compiler/nir/nir.h
@@ -2331,6 +2331,28 @@ nir_after_block_before_jump(nir_block *block)
     }
  }
+static inline nir_cursor
+nir_before_src(nir_src *src, bool is_if_condition)
+{
+   if (is_if_condition) {
+      nir_block *prev_block =
+         nir_cf_node_as_block(nir_cf_node_prev(&src->parent_if->cf_node));
+      assert(!nir_block_ends_in_jump(prev_block));
+      return nir_after_block(prev_block);
+   } else if (src->parent_instr->type == nir_instr_type_phi) {
+      nir_phi_instr *cond_phi = nir_instr_as_phi(src->parent_instr);
+      nir_foreach_phi_src(phi_src, cond_phi) {
+         if (phi_src->src.ssa == src->ssa) {
+            return nir_after_block_before_jump(phi_src->pred);
+         }
+      }
+
+      unreachable("failed to find phi src");
+   } else {
+      return nir_before_instr(src->parent_instr);
+   }
+}
+
  static inline nir_cursor
  nir_before_cf_node(nir_cf_node *node)
  {
diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
index dacf2d6c667..11c6693d302 100644
--- a/src/compiler/nir/nir_opt_if.c
+++ b/src/compiler/nir/nir_opt_if.c
@@ -369,6 +369,76 @@ opt_if_loop_terminator(nir_if *nif)
     return true;
  }
+static void
+replace_if_condition_use_with_const(nir_src *use, void *mem_ctx,
+                                    nir_cursor cursor, unsigned nir_boolean,
+                                    bool if_condition)
+{
+   /* Create const */
+   nir_load_const_instr *load = nir_load_const_instr_create(mem_ctx, 1, 32);
+   load->value.u32[0] = nir_boolean;
+   nir_instr_insert(cursor, &load->instr);
+
+   /* Rewrite use to use const */
+   nir_src new_src = nir_src_for_ssa(&load->def);
+
+   if (if_condition)
+      nir_if_rewrite_condition(use->parent_if, new_src);
+   else
+      nir_instr_rewrite_src(use->parent_instr, use, new_src);
+}
+
+static bool
+evaluate_if_condition(nir_if *nif, nir_cursor cursor, uint32_t *value)
+{
+   nir_block *use_block = nir_cursor_current_block(cursor);
+   if (nir_block_dominates(nir_if_first_then_block(nif), use_block)) {
+      *value = NIR_TRUE;
+      return true;
+   } else if (nir_block_dominates(nir_if_first_else_block(nif), use_block)) {
+      *value = NIR_FALSE;
+      return true;
+   } else {
+      return false;
+   }
+}
+
+static bool
+evaluate_condition_use(nir_if *nif, nir_src *use_src, void *mem_ctx,
+                       bool is_if_condition)
+{
+   bool progress = false;
+
+   uint32_t value;
+   nir_cursor cursor = nir_before_src(use_src, is_if_condition);
+   if (evaluate_if_condition(nif, cursor, &value)) {
+      replace_if_condition_use_with_const(use_src, mem_ctx, cursor, value,
+                                          is_if_condition);
+      progress = true;
+   }
+
+   return progress;
+}
+
+static bool
+opt_if_evaluate_condition_use(nir_if *nif, void *mem_ctx)
+{
+   bool progress = false;
+
+   /* Evaluate any uses of the if condition inside the if branches */
+   assert(nif->condition.is_ssa);
+   nir_foreach_use_safe(use_src, nif->condition.ssa) {
+      progress |= evaluate_condition_use(nif, use_src, mem_ctx, false);
+   }
+
+   nir_foreach_if_use_safe(use_src, nif->condition.ssa) {
+      if (use_src->parent_if != nif)
+         progress |= evaluate_condition_use(nif, use_src, mem_ctx, true);
+   }
+
+   return progress;
+}
+
  static bool
  opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
  {
@@ -402,6 +472,41 @@ opt_if_cf_list(nir_builder *b, struct exec_list *cf_list)
     return progress;
  }
+/**
+ * These optimisations depend on nir_metadata_block_index and therefore must
+ * not do anything to cause the metadata to become invalid.
+ */
+static bool
+opt_if_safe_cf_list(nir_builder *b, struct exec_list *cf_list, void *mem_ctx)
+{
+   bool progress = false;
+   foreach_list_typed(nir_cf_node, cf_node, node, cf_list) {
+      switch (cf_node->type) {
+      case nir_cf_node_block:
+         break;
+
+      case nir_cf_node_if: {
+         nir_if *nif = nir_cf_node_as_if(cf_node);
+         progress |= opt_if_safe_cf_list(b, &nif->then_list, mem_ctx);
+         progress |= opt_if_safe_cf_list(b, &nif->else_list, mem_ctx);
+         progress |= opt_if_evaluate_condition_use(nif, mem_ctx);
+         break;
+      }
+
+      case nir_cf_node_loop: {
+         nir_loop *loop = nir_cf_node_as_loop(cf_node);
+         progress |= opt_if_safe_cf_list(b, &loop->body, mem_ctx);
+         break;
+      }
+
+      case nir_cf_node_function:
+         unreachable("Invalid cf type");
+      }
+   }
+
+   return progress;
+}
+
  bool
  nir_opt_if(nir_shader *shader)
  {
@@ -414,6 +519,14 @@ nir_opt_if(nir_shader *shader)
        nir_builder b;
        nir_builder_init(&b, function->impl);
+ void *mem_ctx = ralloc_parent(function->impl);

Is this the right thing to use for the memory context?  It looks like
most other places use the shader as the memory context.

I had a quick look and to me is seems most places use impl. Maybe you could provide an example? Although I plan on pushing the series as it's been on the list for a long time now and I've finally collected Jason's rb on each patch. As you say its not much difference either way.

If this pass
also used the shader as the memory context, most places could eliminate
the mem_ctx parameter and just use b->shader.  It probably doesn't make
that much difference either way.

+
+      nir_metadata_require(function->impl, nir_metadata_block_index |
+                           nir_metadata_dominance);
+      progress = opt_if_safe_cf_list(&b, &function->impl->body, mem_ctx);
+      nir_metadata_preserve(function->impl, nir_metadata_block_index |
+                            nir_metadata_dominance);
+
        if (opt_if_cf_list(&b, &function->impl->body)) {
           nir_metadata_preserve(function->impl, nir_metadata_none);

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to