On Tue, Jul 10, 2018 at 11:49 PM Timothy Arceri <tarc...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
