Mesa (main): intel/compiler: Don't predicate a WHILE if there is a CONT

[GitLab Mirror]

Module: Mesa
Branch: main
Commit: 4563261ad18c5e82d8fa869e87f938dc123e10b1
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=4563261ad18c5e82d8fa869e87f938dc123e10b1

Author: Ian Romanick <ian.d.romanick at intel.com>
Date:   Mon Sep 20 16:21:45 2021 -0700

intel/compiler: Don't predicate a WHILE if there is a CONT

Previously a predicated BREAK that appeared immediately before the WHILE
would get merged into the WHILE.  This doesn't work if other flow
control (e.g., a CONT) can transfer directly to the WHILE.

On Intel platforms, this fixes the CTS test
dEQP-VK.graphicsfuzz.stable-binarysearch-tree-nested-if-and-conditional.

No shader-db changes on any Intel platform.

When this commit was first created (over a month before it is going to
land), there were some regressions that were prevented by other commits
in MR !13095.  That does not appear to be the case now, so I don't know
what changed.  Basically, the treatment of discard as a combination of
demote and terminate causes additional continues in some loops, and
those continues trigger this bug.  The other commits from that MR
prevent those continues from being generated in the first place.

All Intel platforms had simlar fossil-db results. (Ice Lake shown)
Instructions in all programs: 144419989 -> 144419995 (+0.0%)
SENDs in all programs: 6947332 -> 6947332 (+0.0%)
Loops in all programs: 38277 -> 38277 (+0.0%)
Spills in all programs: 204075 -> 204075 (+0.0%)
Fills in all programs: 319480 -> 319480 (+0.0%)

A few shaders in Doom 2016 were hurt by one instruction each.  It seems
likely that these shaders would have experienced at least some
mis-rendering.

Closes: #4213
Fixes: d13bcdb3a9f ("i965/fs: Extend predicated break pass to predicate WHILE.")
Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14128>

---

 src/intel/compiler/brw_predicated_break.cpp | 77 +++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 5 deletions(-)

diff --git a/src/intel/compiler/brw_predicated_break.cpp b/src/intel/compiler/brw_predicated_break.cpp
index f5f40aaabb3..eaa4320db8f 100644
--- a/src/intel/compiler/brw_predicated_break.cpp
+++ b/src/intel/compiler/brw_predicated_break.cpp
@@ -53,19 +53,79 @@ using namespace brw;
  * and we can remove the BREAK instruction and predicate the WHILE.
  */
 
+#define MAX_NESTING 128
+
+struct loop_continue_tracking {
+   BITSET_WORD has_continue[BITSET_WORDS(MAX_NESTING)];
+   unsigned depth;
+};
+
+static void
+enter_loop(struct loop_continue_tracking *s)
+{
+   s->depth++;
+
+   /* Any loops deeper than that maximum nesting will just re-use the last
+    * flag.  This simplifies most of the code.  MAX_NESTING is chosen to be
+    * large enough that it is unlikely to occur.  Even if it does, the
+    * optimization that uses this tracking is unlikely to make much
+    * difference.
+    */
+   if (s->depth < MAX_NESTING)
+      BITSET_CLEAR(s->has_continue, s->depth);
+}
+
+static void
+exit_loop(struct loop_continue_tracking *s)
+{
+   assert(s->depth > 0);
+   s->depth--;
+}
+
+static void
+set_continue(struct loop_continue_tracking *s)
+{
+   const unsigned i = MIN2(s->depth, MAX_NESTING - 1);
+
+   BITSET_SET(s->has_continue, i);
+}
+
+static bool
+has_continue(const struct loop_continue_tracking *s)
+{
+   const unsigned i = MIN2(s->depth, MAX_NESTING - 1);
+
+   return BITSET_TEST(s->has_continue, i);
+}
+
 bool
 opt_predicated_break(backend_shader *s)
 {
    bool progress = false;
+   struct loop_continue_tracking state = { {0, }, 0 };
 
    foreach_block (block, s->cfg) {
-      if (block->start_ip != block->end_ip)
-         continue;
+      /* DO instructions, by definition, can only be found at the beginning of
+       * basic blocks.
+       */
+      backend_instruction *const do_inst = block->start();
 
-      /* BREAK and CONTINUE instructions, by definition, can only be found at
-       * the ends of basic blocks.
+      /* BREAK, CONTINUE, and WHILE instructions, by definition, can only be
+       * found at the ends of basic blocks.
        */
       backend_instruction *jump_inst = block->end();
+
+      if (do_inst->opcode == BRW_OPCODE_DO)
+         enter_loop(&state);
+
+      if (jump_inst->opcode == BRW_OPCODE_CONTINUE)
+         set_continue(&state);
+      else if (jump_inst->opcode == BRW_OPCODE_WHILE)
+         exit_loop(&state);
+
+      if (block->start_ip != block->end_ip)
+         continue;
+
       if (jump_inst->opcode != BRW_OPCODE_BREAK &&
           jump_inst->opcode != BRW_OPCODE_CONTINUE)
          continue;
@@ -119,13 +179,20 @@ opt_predicated_break(backend_shader *s)
       /* Now look at the first instruction of the block following the BREAK. If
        * it's a WHILE, we can delete the break, predicate the WHILE, and join
        * the two basic blocks.
+       *
+       * This optimization can only be applied if the only instruction that
+       * can transfer control to the WHILE is the BREAK.  If other paths can
+       * lead to the while, the flags may be in an unknown state, and the loop
+       * could terminate prematurely.  This can occur if the loop contains a
+       * CONT instruction.
        */
       bblock_t *while_block = earlier_block->next();
       backend_instruction *while_inst = while_block->start();
 
       if (jump_inst->opcode == BRW_OPCODE_BREAK &&
           while_inst->opcode == BRW_OPCODE_WHILE &&
-          while_inst->predicate == BRW_PREDICATE_NONE) {
+          while_inst->predicate == BRW_PREDICATE_NONE &&
+          !has_continue(&state)) {
          jump_inst->remove(earlier_block);
          while_inst->predicate = jump_inst->predicate;
          while_inst->predicate_inverse = !jump_inst->predicate_inverse;