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

[Kenneth Graunke]

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

> 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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150904/4ddf0327/attachment.sig>