[Mesa-dev] [PATCH 05/11] nir/cf: Alter block successors before adding a fake link.

[Kenneth Graunke]

Consider the case of "while (...) { break }".  Or in NIR:

        block block_0 (0x7ab640):
        ...
        /* succs: block_1 */
        loop {
                block block_1:
                /* preds: block_0 */
                break
                /* succs: block_2 */
        }
        block block_2:

Calling nir_handle_remove_jump(block_1, nir_jump_break) will remove the break.
Unfortunately, it would mangle the predecessors and successors.

Here, block_2->predecessors->entries == 1, so we would create a fake
link, setting block_1->successors[1] = block_2, and adding block_1 to
block_2's predecessor set.  This is illegal: a block cannot specify the
same successor twice.  In particular, adding the predecessor would have
no effect, as it was already present in the set.

We'd then call unlink_block_successors(), which would delete the fake
link and remove block_1 from block_2's predecessor set.  It would then
delete successors[0], and attempt to remove block_1 from block_2's
predecessor set a second time...except that it wouldn't be present,
triggering an assertion failure.

The fix appears to be simple: move the fake link creation after we
recreate the block's successors.  Since we're inspecting "next" later,
If we've created a new infinite
loop, the code following the loop will be dead, and thus 0 predecessors.
So we create a new fake successor.

simply unlink the block's successors and
recreate them to point at the correct blocks first.  Then, add the fake
link.  In the above example, removing the break would cause block_1 to
have itself as a successor (as it becomes an infinite loop), and then

Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
---
 src/glsl/nir/nir_control_flow.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

New patch!

diff --git a/src/glsl/nir/nir_control_flow.c b/src/glsl/nir/nir_control_flow.c
index 2b23f38..a59add5 100644
--- a/src/glsl/nir/nir_control_flow.c
+++ b/src/glsl/nir/nir_control_flow.c
@@ -551,31 +551,29 @@ remove_phi_src(nir_block *block, nir_block *pred)
 static void
 unlink_jump(nir_block *block, nir_jump_type type, bool add_normal_successors)
 {
+   nir_block *next = block->successors[0];
+
    if (block->successors[0])
       remove_phi_src(block->successors[0], block);
    if (block->successors[1])
       remove_phi_src(block->successors[1], block);
 
-   if (type == nir_jump_break) {
-      nir_block *next = block->successors[0];
+   unlink_block_successors(block);
+   if (add_normal_successors)
+      block_add_normal_succs(block);
 
-      if (next->predecessors->entries == 1) {
-         nir_loop *loop =
-            nir_cf_node_as_loop(nir_cf_node_prev(&next->cf_node));
+   if (type == nir_jump_break && next->predecessors->entries == 0) {
+      nir_loop *loop =
+         nir_cf_node_as_loop(nir_cf_node_prev(&next->cf_node));
 
-         /* insert fake link */
-         nir_cf_node *last = nir_loop_last_cf_node(loop);
-         assert(last->type == nir_cf_node_block);
-         nir_block *last_block = nir_cf_node_as_block(last);
+      /* insert fake link */
+      nir_cf_node *last = nir_loop_last_cf_node(loop);
+      assert(last->type == nir_cf_node_block);
+      nir_block *last_block = nir_cf_node_as_block(last);
 
-         last_block->successors[1] = next;
-         block_add_pred(next, last_block);
-      }
+      last_block->successors[1] = next;
+      block_add_pred(next, last_block);
    }
-
-   unlink_block_successors(block);
-   if (add_normal_successors)
-      block_add_normal_succs(block);
 }
 
 void
-- 
2.5.3