[Mesa-stable] [Mesa-dev] [PATCH] spirv: fix visiting inner loops with same break/continue block

[Jason Ekstrand]

On Mon, May 14, 2018 at 1:22 PM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

> On Mon, May 14, 2018 at 1:05 PM, Samuel Pitoiset <
> samuel.pitoiset at gmail.com> wrote:
>
>> We should stop walking through the CFG when the inner loop's
>> break block ends up as the same block as the outer loop's
>> continue block because we are already going to visit it.
>>
>> This fixes the following assertion which ends up by crashing
>> in RADV or ANV:
>>
>> SPIR-V parsing FAILED:
>> In file ../src/compiler/spirv/vtn_cfg.c:381
>> block->node.link.next == NULL
>> 0 bytes into the SPIR-V binary
>>
>> This also fixes a crash with a camera shader from SteamVR.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106090
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106504
>> CC: 18.0 18.1 <mesa-stable at lists.freedesktop.org>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>  src/compiler/spirv/vtn_cfg.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/src/compiler/spirv/vtn_cfg.c b/src/compiler/spirv/vtn_cfg.c
>> index e7d2f9ea61..28554e8c72 100644
>> --- a/src/compiler/spirv/vtn_cfg.c
>> +++ b/src/compiler/spirv/vtn_cfg.c
>> @@ -374,6 +374,13 @@ vtn_cfg_walk_blocks(struct vtn_builder *b, struct
>> list_head *cf_list,
>>           vtn_cfg_walk_blocks(b, &loop->cont_body, new_loop_cont, NULL,
>> NULL,
>>                               new_loop_break, NULL, block);
>>
>> +         /* Stop walking through the CFG when this inner loop's break
>> block
>> +          * ends up as the same block as the outer loop's continue block
>> +          * because we are already going to visit it.
>> +          */
>> +         if (new_loop_break == loop_cont)
>> +            return;
>>
>
> I think this is mostly correct.  However, I think what we really want is
> to call vtn_get_branch_type() and bail if the returnd branch type is not
> vtn_branch_type_none.  Possibly with an assert like we have in switch case
> handling.
>

In particular, I think loop continues and none are probably the only valid
branch types.  It's possible that a switch case fall-through would also be
valid but I haven't thought about it long enough.  That should be
considered.

--Jason
>
>
>> +
>>           block = new_loop_break;
>>           continue;
>>        }
>> --
>> 2.17.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20180514/61f0469f/attachment-0001.html>