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

[Connor Abbott]

On Sat, Sep 5, 2015 at 2:31 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Friday, September 04, 2015 11:56:29 AM Connor Abbott wrote:
>> 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
>
> FYI, here are the Piglit tests that break (from nir/cf: Fix
> unlink_block_successors to actually unlink the second one.)
>
> glslparsertest/shaders/correctfull.frag
> shaders/dead-code-break-interaction
> shaders/glsl-link-bug30552
> shaders/loopfunc
> spec/glsl-1.30/compiler/switch-statement/switch-case-const-int-expression.vert
> spec/glsl-1.30/compiler/switch-statement/switch-case-const-int.vert
> spec/glsl-1.30/compiler/switch-statement/switch-case-fallthrough.vert
> spec/glsl-1.30/compiler/switch-statement/switch-case-statement.vert
> spec/glsl-1.30/compiler/switch-statement/switch-default.vert
> spec/glsl-1.30/compiler/switch-statement/switch-expression-const-int.vert
> spec/glsl-1.30/compiler/switch-statement/switch-expression-in-int.vert
> spec/glsl-1.30/compiler/switch-statement/switch-expression-uniform-int.vert
> spec/glsl-1.30/compiler/switch-statement/switch-expression-var-int.vert
> spec/glsl-1.30/compiler/switch-statement/switch-nested-break.vert
> spec/glsl-1.30/compiler/switch-statement/switch-nested-loop.vert
> spec/glsl-1.30/compiler/switch-statement/switch-nested-switch.vert

In that case, I would check that this is true:

assert(last->successors[0] ==
nir_cf_node_as_block(nir_loop_first_cf_node(loop)));
assert(last->successors[1] == NULL);

That is, the last block of the loop should only have one successor
which is the beginning of the loop. You can also check that
cf_node_prev(&next->cf_node) is actually a loop (it seems I was lazy
with that).

Note that next is the block after the loop, via how we found the loop
in the first place, so if "loop" is actually a loop and those
assertions hold, then there must be some linked-list corruption going
on.

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