nir: fix crash in loop unroll corner case

Submitted by Timothy Arceri on March 26, 2018, 1:32 a.m.

Details

Message ID 20180326013230.3320-1-tarceri@itsqueeze.com
State Accepted
Headers show
Series "nir: fix crash in loop unroll corner case" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Timothy Arceri March 26, 2018, 1:32 a.m.
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(-)

Patch hide | download patch | download mbox

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.
  *
  * 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));
+      }
+   }
 
+   /* 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);

Comments

On Sun, Mar 25, 2018 at 6:32 PM, Timothy Arceri <tarceri@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>