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