[Mesa-dev] [PATCH] nir: fix crash in loop unroll corner case

[Jason Ekstrand]

On Sun, Mar 25, 2018 at 6:32 PM, Timothy Arceri <tarceri at itsqueeze.com>
wrote:

> When an if nesting inside anouther if is optimised away we can
> end up with a loop terminator and following block that looks like
> this:
>
>         if ssa_596 {
>                 block block_5:
>                 /* preds: block_4 */
>                 vec1 32 ssa_601 = load_const (0xffffffff /* -nan */)
>                 break
>                 /* succs: block_8 */
>         } else {
>                 block block_6:
>                 /* preds: block_4 */
>                 /* succs: block_7 */
>         }
>         block block_7:
>         /* preds: block_6 */
>         vec1 32 ssa_602 = phi block_6: ssa_552
>         vec1 32 ssa_603 = phi block_6: ssa_553
>         vec1 32 ssa_604 = iadd ssa_551, ssa_66
>
> The problem is the phis. Loop unrolling expects the last block in
> the loop to be empty once we splice the instructions in the last
> block into the continue branch. The problem is we cant move phis
> so here we lower the phis to regs when preparing the loop for
> unrolling. As it could be possible to have multiple additional
> blocks/ifs following the terminator we just convert all phis at
> the top level of the loop body for simplicity.
>
> We also add some comments to loop_prepare_for_unroll() while we
> are here.
>
> Fixes: 51daccb289eb "nir: add a loop unrolling pass"
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105670
> ---
>  src/compiler/nir/nir_opt_loop_unroll.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_loop_unroll.c
> b/src/compiler/nir/nir_opt_loop_unroll.c
> index 79d04f978bc..85df3097264 100644
> --- a/src/compiler/nir/nir_opt_loop_unroll.c
> +++ b/src/compiler/nir/nir_opt_loop_unroll.c
> @@ -37,10 +37,10 @@
>  #define LOOP_UNROLL_LIMIT 26
>
>  /* Prepare this loop for unrolling by first converting to lcssa and then
> - * converting the phis from the loops first block and the block that
> follows
> - * the loop into regs.  Partially converting out of SSA allows us to
> unroll
> - * the loop without having to keep track of and update phis along the way
> - * which gets tricky and doesn't add much value over conveting to regs.
> + * converting the phis from the top level of the loop body to regs.
> + * Partially converting out of SSA allows us to unroll the loop without
> having
> + * to keep track of and update phis along the way which gets tricky and
> + * doesn't add much value over conveting to regs.
>

converting


>   *
>   * The loop may have a continue instruction at the end of the loop which
> does
>   * nothing.  Once we're out of SSA, we can safely delete it so we don't
> have
> @@ -51,13 +51,20 @@ loop_prepare_for_unroll(nir_loop *loop)
>  {
>     nir_convert_loop_to_lcssa(loop);
>
> -   nir_lower_phis_to_regs_block(nir_loop_first_block(loop));
> +   /* Lower phis at the top level of the loop body */
> +   foreach_list_typed_safe(nir_cf_node, node, node, &loop->body) {
> +      if (nir_cf_node_block == node->type) {
> +         nir_lower_phis_to_regs_block(nir_cf_node_as_block(node));
> +      }
> +   }
>

This is a bit of a big hammer and I don't really like it for that but it
does work.  We could predicate it on block->predecessors->entries == 1 or
we could try and do something even more clever.  That said, this is tricky
and what you've done is robust so I'm ok with it as-is.

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


> +   /* Lower phis after the loop */
>     nir_block *block_after_loop =
>        nir_cf_node_as_block(nir_cf_node_next(&loop->cf_node));
>
>     nir_lower_phis_to_regs_block(block_after_loop);
>
> +   /* Remove continue if its the last instruction in the loop */
>     nir_instr *last_instr = nir_block_last_instr(nir_loop_
> last_block(loop));
>     if (last_instr && last_instr->type == nir_instr_type_jump) {
>        assert(nir_instr_as_jump(last_instr)->type == nir_jump_continue);
> --
> 2.14.3
>
> _______________________________________________
> 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/20180326/95a0a0ac/attachment.html>