<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Tue, Jul 10, 2018 at 11:49 PM Timothy Arceri <<a href="mailto:tarceri@itsqueeze.com">tarceri@itsqueeze.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The innermost check was added to stop us from unrolling multiple<br>
loops in a single pass, and to stop outer loops from unrolling.<br></blockquote><div><br></div><div>Do you mean "multiple nested loops in a single pass..."?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
When we successfully unroll a loop we need to run the analysis<br>
pass again before deciding if we want to go ahead an unroll a<br>
second loop.<br></blockquote><div><br></div><div>"unroll its parent loop" or perhaps "unroll a loop containing it"?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
However the logic was flawed because it never tried to unroll any<br>
nested loops other than the first innermost loop it found.<br>
If this innermost loop is not unrolled we end up skipping all<br>
other nested loops.<br>
<br>
No change to shader-db. Unrolls a loop in a shader from the game<br>
Prey when running on DXVK.<br>
---<br>
 src/compiler/nir/nir_opt_loop_unroll.c | 31 ++++++++++++++------------<br>
 1 file changed, 17 insertions(+), 14 deletions(-)<br>
<br>
diff --git a/src/compiler/nir/nir_opt_loop_unroll.c b/src/compiler/nir/nir_opt_loop_unroll.c<br>
index 955dfede694..f31f84bee76 100644<br>
--- a/src/compiler/nir/nir_opt_loop_unroll.c<br>
+++ b/src/compiler/nir/nir_opt_loop_unroll.c<br>
@@ -483,7 +483,7 @@ is_loop_small_enough_to_unroll(nir_shader *shader, nir_loop_info *li)<br>
 }<br>
<br>
 static bool<br>
-process_loops(nir_shader *sh, nir_cf_node *cf_node, bool *innermost_loop)<br>
+process_loops(nir_shader *sh, nir_cf_node *cf_node, bool *has_nested_loop)<br>
 {<br>
    bool progress = false;<br>
    nir_loop *loop;<br>
@@ -494,32 +494,33 @@ process_loops(nir_shader *sh, nir_cf_node *cf_node, bool *innermost_loop)<br>
    case nir_cf_node_if: {<br>
       nir_if *if_stmt = nir_cf_node_as_if(cf_node);<br>
       foreach_list_typed_safe(nir_cf_node, nested_node, node, &if_stmt->then_list)<br>
-         progress |= process_loops(sh, nested_node, innermost_loop);<br>
+         progress |= process_loops(sh, nested_node, has_nested_loop);<br>
       foreach_list_typed_safe(nir_cf_node, nested_node, node, &if_stmt->else_list)<br>
-         progress |= process_loops(sh, nested_node, innermost_loop);<br>
+         progress |= process_loops(sh, nested_node, has_nested_loop);<br>
       return progress;<br>
    }<br>
    case nir_cf_node_loop: {<br>
+      *has_nested_loop = false;<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
       loop = nir_cf_node_as_loop(cf_node);<br>
       foreach_list_typed_safe(nir_cf_node, nested_node, node, &loop->body)<br>
-         progress |= process_loops(sh, nested_node, innermost_loop);<br>
+         progress |= process_loops(sh, nested_node, has_nested_loop);<br>
+ <br></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
       break;<br>
    }<br>
    default:<br>
       unreachable("unknown cf node type");<br>
    }<br>
<br>
-   if (*innermost_loop) {<br>
-      /* Don't attempt to unroll outer loops or a second inner loop in<br>
-       * this pass wait until the next pass as we have altered the cf.<br>
-       */<br>
-      *innermost_loop = false;<br>
+   /* Don't attempt to unroll a second inner loop in this pass, wait until the<br>
+    * next pass as we have altered the cf.<br>
+    */<br>
+   if (!progress) {<br>
<br>
-      if (loop->info->limiting_terminator == NULL)<br>
-         return progress;<br>
+      if (*has_nested_loop || loop->info->limiting_terminator == NULL)<br>
+         goto exit;<br>
<br>
       if (!is_loop_small_enough_to_unroll(sh, loop->info))<br>
-         return progress;<br>
+         goto exit;<br>
<br>
       if (loop->info->is_trip_count_known) {<br>
          simple_unroll(loop);<br>
@@ -555,6 +556,8 @@ process_loops(nir_shader *sh, nir_cf_node *cf_node, bool *innermost_loop)<br>
       }<br>
    }<br>
<br>
+exit:<br>
+   *has_nested_loop = true;<br>
    return progress;<br>
 }<br>
<br>
@@ -567,9 +570,9 @@ nir_opt_loop_unroll_impl(nir_function_impl *impl,<br>
    nir_metadata_require(impl, nir_metadata_block_index);<br>
<br>
    foreach_list_typed_safe(nir_cf_node, node, node, &impl->body) {<br>
-      bool innermost_loop = true;<br>
+      bool has_nested_loop = false;<br>
       progress |= process_loops(impl->function->shader, node,<br>
-                                &innermost_loop);<br>
+                                &has_nested_loop);<br>
    }<br>
<br>
    if (progress)<br>
-- <br>
2.17.1<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</blockquote></div></div>