[Mesa-dev] [PATCH 1/5] nir: Don't insert a fake link if unnecessary.

[Connor Abbott]

On Thu, Sep 3, 2015 at 2:32 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> Prevents regressions in ~128 tests when fixing unlink_block_successors
> in the next commit.
>
> XXX: Zero thought has been put into whether this is the right solution

I'm confused as to how this can happen. The fake link is only for the
situation where we have an infinite loop:

while(true) {
   ...
   if (false)
      break;
}

Say that we're doing dead control flow elimination, and we want to
remove the break. Now, we're left with an infinite loop:

while (true) {
   ...
}

But now, the code after the loop is unreachable, and the blocks after
it get bogus dominance tree information, which causes some piglit
parser tests to crash. The hack I added was to create a "fake" link
from the last block in the loop to the block after the loop. Now,
dominance analysis (and whatever else) thinks there's a way to get out
of the loop, which keeps stuff like to-SSA from crashing.
block->successors[0] should still point to the first block in the
loop, and block->successors[1] should point to the block after the
loop, so they shouldn't be the same. Maybe there's another bug that
this is hiding?

P.S. now that we have dead control flow elimination, maybe we can
simply mark all code after an infinite loop as unreachable, and make
sure that we run it before we try to use to-SSA or anything else that
depends on the dominance tree. After all, there's a similar situation
with

if (foo)
   break;
else
   continue;

and there, we were relying on GLSL IR to optimize away everything after the if.

>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/glsl/nir/nir_control_flow.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> The problem is that this fake link causes the same block to be
> block->successors[0] and block->successors[1]...
>
> diff --git a/src/glsl/nir/nir_control_flow.c b/src/glsl/nir/nir_control_flow.c
> index 5c03375..d0b2a10 100644
> --- a/src/glsl/nir/nir_control_flow.c
> +++ b/src/glsl/nir/nir_control_flow.c
> @@ -542,13 +542,15 @@ void unlink_jump(nir_block *block, nir_jump_type type)
>           nir_loop *loop =
>              nir_cf_node_as_loop(nir_cf_node_prev(&next->cf_node));
>
> -         /* insert fake link */
> +         /* insert fake link if necessary */
>           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);
> +         if (last_block->successors[0] != next) {
> +            last_block->successors[1] = next;
> +            block_add_pred(next, last_block);
> +         }
>        }
>     }
>
> --
> 2.5.0
>