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

[Connor Abbott]

On Wed, Sep 9, 2015 at 1:37 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Saturday, September 05, 2015 11:10:58 AM Connor Abbott wrote:
>> 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:
>> >> I'm confused as to how this can happen. The fake link is only for the
>> >> situation where we have an infinite loop:
>
> Okay...
>
>> 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).
>
> So, looking at tests/shaders/loopfunc.shader_test, I'm seeing the
> following NIR before opt_dead_cf() runs:
>
> decl_var shader_in  vec4 gl_Vertex (0, 0)
> decl_var shader_out  vec4 gl_FrontColor (1, 0)
> decl_var shader_out  vec4 gl_Position (0, 0)
> decl_overload main returning void
>
> impl main {
>         decl_var vec4 const_temp
>         decl_var vec4 gl_Position at 0
>         decl_var vec4 gl_FrontColor at 1
>         block block_0 (0x7ab640):
>         /* preds: */
>         vec4 ssa_2 = load_const (0x00000000 /* 0.000000 */, 0x3f800000 /* 1.000000 */, 0x00000000 /* 0.000000 */, 0x3f800000 /* 1.000000 */)
>         /* succs: block_1 (0x7ab880) */
>         loop {
>                 block block_1 (0x7ab880):
>                 /* preds: block_0 (0x7ab640) */
>                 break
>                 /* succs: block_2 (0x7a9db0) */
>         }
>         block block_2 (0x7a9db0):
>         /* preds: block_1 (0x7ab880) */
>         vec4 ssa_4 = intrinsic load_var () (gl_Vertex) ()
>         intrinsic store_var (ssa_4) (gl_Position) ()
>         intrinsic store_var (ssa_2) (gl_FrontColor) ()
>         /* succs: block_3 (0x7ab760) */
>         block block_3:
> }
>
> Because the loop contains an unconditional break, the block does only
> have one successor - but to the block after the loop.  This seems
> reasonable...but it's not an infinite loop.
>
> So, we extract the loop, delete the extracted section...which calls
> unlink_jump on the "break".  This gives block_1 a fake link, so both
> successors then point at block_2, and things go very wrong.
>
> Does this seem like valid NIR to you, Connor?  If so, I think we need
> to adjust the fake-link conditions to account for this...

Oh, ok, I see... it wasn't handling correctly the case where the loop
ends in a break, where we have to add both new edges (i.e. the edge to
the beginning of the loop and the fake edge to after the loop) *after*
unlinking the break. Sigh... so it was broken even without the
fake-edge code, but doing this papered over the problem. Does that
seem right?