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

[Timothy Arceri]

On 22/07/18 19:32, Jason Ekstrand wrote:
> On Tue, Jul 10, 2018 at 11:49 PM Timothy Arceri <tarceri at itsqueeze.com 
> <mailto: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..."?

I believe the above description is worded correctly as it also stops 
multiple top level loops from unrolling (although I admit innermost_loop 
was not the best choice of name). If I recall correctly deleting a loop 
from control flow meant we couldn't unroll a loop directly following 
that loop because the cf has been altered so much that the 
process_loops() calls would loose their place making it unsafe to continue.

> 
>     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"?

No as above we only unroll a single loop at a time. I believe we 
discussed this in review of the original series.

> 
>     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.

Yeah I think so will send a new patch.

> 
>             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 <mailto:mesa-dev at lists.freedesktop.org>
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>