[Mesa-dev] [PATCH 03/10] nir: allow more nested loops to be unrolled

[Jason Ekstrand]

On Tue, Jul 10, 2018 at 11:49 PM Timothy Arceri <tarceri at itsqueeze.com>
wrote:

> The innermost check was added to stop us from unrolling multiple
> loops in a single pass, and to stop outer loops from unrolling.
>

Do you mean "multiple nested loops in a single pass..."?


> When we successfully unroll a loop we need to run the analysis
> pass again before deciding if we want to go ahead an unroll a
> second loop.
>

"unroll its parent loop" or perhaps "unroll a loop containing it"?


> However the logic was flawed because it never tried to unroll any
> nested loops other than the first innermost loop it found.
> If this innermost loop is not unrolled we end up skipping all
> other nested loops.
>
> No change to shader-db. Unrolls a loop in a shader from the game
> Prey when running on DXVK.
> ---
>  src/compiler/nir/nir_opt_loop_unroll.c | 31 ++++++++++++++------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/src/compiler/nir/nir_opt_loop_unroll.c
> b/src/compiler/nir/nir_opt_loop_unroll.c
> index 955dfede694..f31f84bee76 100644
> --- a/src/compiler/nir/nir_opt_loop_unroll.c
> +++ b/src/compiler/nir/nir_opt_loop_unroll.c
> @@ -483,7 +483,7 @@ is_loop_small_enough_to_unroll(nir_shader *shader,
> nir_loop_info *li)
>  }
>
>  static bool
> -process_loops(nir_shader *sh, nir_cf_node *cf_node, bool *innermost_loop)
> +process_loops(nir_shader *sh, nir_cf_node *cf_node, bool *has_nested_loop)
>  {
>     bool progress = false;
>     nir_loop *loop;
> @@ -494,32 +494,33 @@ process_loops(nir_shader *sh, nir_cf_node *cf_node,
> bool *innermost_loop)
>     case nir_cf_node_if: {
>        nir_if *if_stmt = nir_cf_node_as_if(cf_node);
>        foreach_list_typed_safe(nir_cf_node, nested_node, node,
> &if_stmt->then_list)
> -         progress |= process_loops(sh, nested_node, innermost_loop);
> +         progress |= process_loops(sh, nested_node, has_nested_loop);
>        foreach_list_typed_safe(nir_cf_node, nested_node, node,
> &if_stmt->else_list)
> -         progress |= process_loops(sh, nested_node, innermost_loop);
> +         progress |= process_loops(sh, nested_node, has_nested_loop);
>        return progress;
>     }
>     case nir_cf_node_loop: {
> +      *has_nested_loop = false;
>

If I'm understanding what you're trying to do correctly, I think this patch
is correct but the usage of the recursive boolean is driving me nuts.  How
about we rename the function parameter to has_nested_loop_out and declare a
local variable has_nested_loop which we initialize to false.  Then we would
pass the function parameter version straight through in the if case and, in
the loop case we would set *has_nested_loop_out = true and then pass the
local one into the recursive call.  Would that make more sense?  Recursion
is hard.


>        loop = nir_cf_node_as_loop(cf_node);
>        foreach_list_typed_safe(nir_cf_node, nested_node, node, &loop->body)
> -         progress |= process_loops(sh, nested_node, innermost_loop);
> +         progress |= process_loops(sh, nested_node, has_nested_loop);
> +
>
       break;
>     }
>     default:
>        unreachable("unknown cf node type");
>     }
>
> -   if (*innermost_loop) {
> -      /* Don't attempt to unroll outer loops or a second inner loop in
> -       * this pass wait until the next pass as we have altered the cf.
> -       */
> -      *innermost_loop = false;
> +   /* Don't attempt to unroll a second inner loop in this pass, wait
> until the
> +    * next pass as we have altered the cf.
> +    */
> +   if (!progress) {
>
> -      if (loop->info->limiting_terminator == NULL)
> -         return progress;
> +      if (*has_nested_loop || loop->info->limiting_terminator == NULL)
> +         goto exit;
>
>        if (!is_loop_small_enough_to_unroll(sh, loop->info))
> -         return progress;
> +         goto exit;
>
>        if (loop->info->is_trip_count_known) {
>           simple_unroll(loop);
> @@ -555,6 +556,8 @@ process_loops(nir_shader *sh, nir_cf_node *cf_node,
> bool *innermost_loop)
>        }
>     }
>
> +exit:
> +   *has_nested_loop = true;
>     return progress;
>  }
>
> @@ -567,9 +570,9 @@ nir_opt_loop_unroll_impl(nir_function_impl *impl,
>     nir_metadata_require(impl, nir_metadata_block_index);
>
>     foreach_list_typed_safe(nir_cf_node, node, node, &impl->body) {
> -      bool innermost_loop = true;
> +      bool has_nested_loop = false;
>        progress |= process_loops(impl->function->shader, node,
> -                                &innermost_loop);
> +                                &has_nested_loop);
>     }
>
>     if (progress)
> --
> 2.17.1
>
> _______________________________________________
> 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/20180722/aa1ced19/attachment-0001.html>