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

[Connor Abbott]

On Tue, Sep 22, 2015 at 11:01 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> 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 */

I was a little terse here with this comment, and based on how long it
took to actually fix this, there seems to be a lot going on that one
could easily miss from reading the code. The commit message has a
pretty good summary of the problem, but perhaps we should at least add
a comment here explaining that we can't do this until now since if the
break is at the end of the loop then last_block == block and we'd be
inserting the same successor twice and generally screwing things up.
Other than that, this patch and the previous one are

Reviewed-by: Connor Abbott <cwabbott0 at gmail.com>

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