[Mesa-dev] [PATCH] nir/dead_cf: also remove useless ifs

Jason Ekstrand jason at jlekstrand.net
Sat Mar 17 17:08:47 UTC 2018


On March 16, 2018 18:25:14 Caio Marcelo de Oliveira Filho 
<caio.oliveira at intel.com> wrote:

> Generalize the code for remove dead loops to also remove dead if
> nodes. The conditions are the same in both cases, if the node (and
> it's children) don't have side-effects AND the nodes after it don't
> use the values produced by the node.
>
> The only difference is when evaluating side effects: loops consider
> only return jumps as a side-effect -- they can stop execution of nodes
> after it; if nodes should consider all kinds of jumps (return, break,
> continue) since all of them can cause execution of nodes after it to
> be skipped.
>
> After this patch, empty ifs (those which both then and else blocks are
> empty) will be removed by nir_opt_dead_cf.
>
> No changes to shader-db results, in part because the removal of empty
> ifs is currently covered by nir_opt_peephole_select.
> ---
>
> The motivation is a lowering pass I'm working for anv, that clones the
> shader and makes certain changes to it, for which I want to use dead
> control flow elimination.
>
>  src/compiler/nir/nir_opt_dead_cf.c | 62 +++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 24 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_dead_cf.c 
> b/src/compiler/nir/nir_opt_dead_cf.c
> index 53c92ecc28..15d219ac9d 100644
> --- a/src/compiler/nir/nir_opt_dead_cf.c
> +++ b/src/compiler/nir/nir_opt_dead_cf.c
> @@ -60,12 +60,12 @@
>   * }
>   * ...
>   *
> - * Finally, we also handle removing useless loops, i.e. loops with no side
> - * effects and without any definitions that are used elsewhere. This case is a
> - * little different from the first two in that the code is actually run (it
> - * just never does anything), but there are similar issues with needing to
> - * be careful with restarting after deleting the cf_node (see dead_cf_list())
> - * so this is a convenient place to remove them.
> + * Finally, we also handle removing useless loops and ifs, i.e. loops and ifs
> + * with no side effects and without any definitions that are used
> + * elsewhere. This case is a little different from the first two in that the
> + * code is actually run (it just never does anything), but there are similar
> + * issues with needing to be careful with restarting after deleting the
> + * cf_node (see dead_cf_list()) so this is a convenient place to remove them.
>   */
>
>  static void
> @@ -135,18 +135,26 @@ opt_constant_if(nir_if *if_stmt, bool condition)
>  static bool
>  cf_node_has_side_effects(nir_cf_node *node)
>  {
> +   const bool inside_if_stmt = node->type == nir_cf_node_if;

I think "inside_loop" would be the better condition since it's the more 
restrictive.  The real thing here is that you can ignore break and continue 
if your inside a loop.

>     nir_foreach_block_in_cf_node(block, node) {

If you did something here like

bool inside_loop = node->type == nir_cf_node_loop;
for (nir_cf_node *n = &block->cf_node; n != node; n = n->parent) {
   if (n->type == nir_cf_node_loop)
      inside_loop = true;
}

That should get rid of the issue you cited below where it's too 
conservative about loops inside ifs.

>        nir_foreach_instr(instr, block) {
>           if (instr->type == nir_instr_type_call)
>              return true;
>
> -         /* Return instructions can cause us to skip over other side-effecting
> -          * instructions after the loop, so consider them to have side effects
> -          * here.
> +         /* For loops, return instructions can cause us to skip over other
> +          * side-effecting instructions after the loop, so consider them to
> +          * have side effects here.
> +          *
> +          * For ifs, all jumps can cause skip over other side effect
> +          * instructions to be skipped.
> +          *
> +          * TODO: Fix the traversal to be less conservative. Loops inside if
> +          * can break without necessarily causing side effects.
>            */
>
>           if (instr->type == nir_instr_type_jump &&
> -             nir_instr_as_jump(instr)->type == nir_jump_return)
> +             (inside_if_stmt || nir_instr_as_jump(instr)->type == 
> nir_jump_return))
>              return true;
>
>           if (instr->type != nir_instr_type_intrinsic)
> @@ -171,36 +179,37 @@ def_not_live_out(nir_ssa_def *def, void *state)
>  }
>
>  /*
> - * Test if a loop is dead. A loop is dead if:
> + * Test if a loop node or if node is dead. Such nodes are dead if:
>   *
>   * 1) It has no side effects (i.e. intrinsics which could possibly affect the
>   * state of the program aside from producing an SSA value, indicated by a lack
>   * of NIR_INTRINSIC_CAN_ELIMINATE).
>   *
> - * 2) It has no phi nodes after it, since those indicate values inside the
> - * loop being used after the loop.
> + * 2) It has no phi instructions after it, since those indicate values inside
> + * the node are being used after the node.
> + *
> + * 3) None of the values defined inside the node is used outside the node,
> + * i.e. none of the definitions that dominate the node exit are used outside.
>   *
> - * 3) If there are no phi nodes after the loop, then the only way a value
> - * defined inside the loop can be used outside the loop is if its definition
> - * dominates the block after the loop. If none of the definitions that
> - * dominate the loop exit are used outside the loop, then the loop is dead
> - * and it can be deleted.
> + * If those conditions hold, then the node is dead and can be deleted.
>   */
>
>  static bool
> -loop_is_dead(nir_loop *loop)
> +node_is_dead(nir_cf_node *node)
>  {
> -   nir_block *before = nir_cf_node_as_block(nir_cf_node_prev(&loop->cf_node));
> -   nir_block *after = nir_cf_node_as_block(nir_cf_node_next(&loop->cf_node));
> +   assert(node->type == nir_cf_node_loop || node->type == nir_cf_node_if);
> +
> +   nir_block *before = nir_cf_node_as_block(nir_cf_node_prev(node));
> +   nir_block *after = nir_cf_node_as_block(nir_cf_node_next(node));
>
>     if (!exec_list_is_empty(&after->instr_list) &&
>         nir_block_first_instr(after)->type == nir_instr_type_phi)
>        return false;
>
> -   if (cf_node_has_side_effects(&loop->cf_node))
> +   if (cf_node_has_side_effects(node))
>        return false;
>
> -   nir_function_impl *impl = nir_cf_node_get_function(&loop->cf_node);
> +   nir_function_impl *impl = nir_cf_node_get_function(node);
>     nir_metadata_require(impl, nir_metadata_live_ssa_defs |
>                                nir_metadata_dominance);
>
> @@ -220,6 +229,11 @@ dead_cf_block(nir_block *block)
>  {
>     nir_if *following_if = nir_block_get_following_if(block);
>     if (following_if) {
> +      if (node_is_dead(&following_if->cf_node)) {
> +         nir_cf_node_remove(&following_if->cf_node);
> +         return true;
> +      }
> +
>        nir_const_value *const_value =
>           nir_src_as_const_value(following_if->condition);
>
> @@ -234,7 +248,7 @@ dead_cf_block(nir_block *block)
>     if (!following_loop)
>        return false;
>
> -   if (!loop_is_dead(following_loop))
> +   if (!node_is_dead(&following_loop->cf_node))
>        return false;
>
>     nir_cf_node_remove(&following_loop->cf_node);
> --
> 2.16.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev




More information about the mesa-dev mailing list