[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