[Mesa-dev] [PATCH v4 1/7] nir: evaluate if condition uses inside the if branches

[Ian Romanick]

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.  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);
>