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

[Jason Ekstrand]

Both are

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

On Mon, Mar 19, 2018 at 4:34 PM, 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 outside loops 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.
>
> It caused no change to shader-db, in part because the removal of empty
> ifs is currently covered by nir_opt_peephole_select.
>
> v2: Improve the identification of cases where break/continue can cause
>     side-effects. (Jason)
>
> v3: Move code comment changes to a different patch. (Jason)
> ---
>  src/compiler/nir/nir_opt_dead_cf.c | 44 ++++++++++++++++++++++++++----
> --------
>  1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_dead_cf.c
> b/src/compiler/nir/nir_opt_dead_cf.c
> index b15dfdd0c9..a652bcd99b 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
> @@ -136,6 +136,12 @@ static bool
>  cf_node_has_side_effects(nir_cf_node *node)
>  {
>     nir_foreach_block_in_cf_node(block, node) {
> +      bool inside_loop = node->type == nir_cf_node_loop;
> +      for (nir_cf_node *n = &block->cf_node; !inside_loop && n != node; n
> = n->parent) {
> +         if (n->type == nir_cf_node_loop)
> +            inside_loop = true;
> +      }
> +
>        nir_foreach_instr(instr, block) {
>           if (instr->type == nir_instr_type_call)
>              return true;
> @@ -143,10 +149,13 @@ cf_node_has_side_effects(nir_cf_node *node)
>           /* Return instructions can cause us to skip over other
> side-effecting
>            * instructions after the loop, so consider them to have side
> effects
>            * here.
> +          *
> +          * When the block is not inside a loop, break and continue might
> also
> +          * cause a skip.
>            */
>
>           if (instr->type == nir_instr_type_jump &&
> -             nir_instr_as_jump(instr)->type == nir_jump_return)
> +             (!inside_loop || nir_instr_as_jump(instr)->type ==
> nir_jump_return))
>              return true;
>
>           if (instr->type != nir_instr_type_intrinsic)
> @@ -171,7 +180,7 @@ def_not_live_out(nir_ssa_def *def, void *state)
>  }
>
>  /*
> - * Test if a loop is dead. A loop node 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
> @@ -187,19 +196,21 @@ def_not_live_out(nir_ssa_def *def, void *state)
>   */
>
>  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);
>
> @@ -219,6 +230,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);
>
> @@ -233,7 +249,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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180319/a56d5c7e/attachment-0001.html>