spirv: fix visiting inner loops with same break/continue block

Submitted by Samuel Pitoiset on May 14, 2018, 8:05 p.m.

Details

Message ID 20180514200545.2770-1-samuel.pitoiset@gmail.com
State New
Headers show
Series "spirv: fix visiting inner loops with same break/continue block" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset May 14, 2018, 8:05 p.m.
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@lists.freedesktop.org>
Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 src/compiler/spirv/vtn_cfg.c | 7 +++++++
 1 file changed, 7 insertions(+)

Patch hide | download patch | download mbox

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;
+
          block = new_loop_break;
          continue;
       }

Comments

On Mon, May 14, 2018 at 1:05 PM, Samuel Pitoiset <samuel.pitoiset@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@lists.freedesktop.org>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@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.

--Jason


> +
>           block = new_loop_break;
>           continue;
>        }
> --
> 2.17.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Mon, May 14, 2018 at 1:22 PM, Jason Ekstrand <jason@jlekstrand.net>
wrote:

> On Mon, May 14, 2018 at 1:05 PM, Samuel Pitoiset <
> samuel.pitoiset@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@lists.freedesktop.org>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>