[Mesa-dev] [PATCH] spirv: Handle patch decorations up-front

[Kenneth Graunke]

On Wednesday, January 11, 2017 6:32:34 PM PST Jason Ekstrand wrote:
> Once again, SPIR-V is insane... It allows you to place "patch"
> decorations on structure members.  Presumably, this is so that you can
> do something such as
> 
> out struct S {
>    layout(location = 0) patch vec4 thing1;
>    layout(location = 0) vec4 thing2;
> } str;
> 
> And have your I/O "nicely" organized.  While this is a bit silly, it's
> allowed and well-defined so whatever.  Where it really gets interesting
> is when you have an array of struct.  SPIR-V says nothing about not
> allowing you to have those qualifiers on the members of a struct that's
> inside an array and GLSLang does this.  Specifically, if you have
> 
> layout(location = 0) out patch struct S {
>    vec4 thing1;
>    vec4 thing2;
> } str[2];
> 
> then GLSLang will place the "patch" decorations on the struct members.
> This is ridiculous there is no way that having some of them be patch and
> some not would be well-defined given that patch and non-patch outputs
> are in effectively different storage classes.  This commit moves around
> the way we handle the "patch" decoration so that we can detect even the
> crazy cases and handle them.
> 
> Fixes: dEQP-VK.tessellation.user_defined_io.per_patch_block_array.*
> ---
>  src/compiler/spirv/vtn_variables.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c
> index 3ecb54f..91e5b13 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1359,8 +1359,29 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode,
>  
>        case vtn_variable_mode_input:
>        case vtn_variable_mode_output: {
> +         /* In order to know whether or not we're a per-vertex inout, we need
> +          * the patch qualifier.  This means walking the variable decorations
> +          * early before we actually create any variables.  Not a big deal.
> +          *
> +          * GLSLang really likes to place decorations in the most interior
> +          * thing it possibly can.  In particular, if you have a struct, it
> +          * will place the patch decorations on the struct members.  This
> +          * should be handled by the variable splitting below just fine.
> +          *
> +          * If you have an array-of-struct, things get even more wierd as it

weird

> +          * will place the patch decorations on the struct even though it's
> +          * inside an array and some of the members being patch and others not
> +          * makes no sense whatsoever.  Since the only sensible thing is for
> +          * it to be all or nothing, we'll call it patch if any of the members
> +          * are declared patch.
> +          */

I wonder if we could emit a warning if they don't match.  That would be
nice, although not essential...

It would be nice if SPIR-V on glslang were less crazy in this regard
but I don't expect either of those to realistically happen.  I think
your approach is reasonable.

Thank you so much for fixing this for me.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

>           var->patch = false;
>           vtn_foreach_decoration(b, val, var_is_patch_cb, &var->patch);
> +         if (glsl_type_is_array(var->type->type) &&
> +             glsl_type_is_struct(without_array->type)) {
> +            vtn_foreach_decoration(b, without_array->val,
> +                                   var_is_patch_cb, &var->patch);
> +         }
>  
>           /* For inputs and outputs, we immediately split structures.  This
>            * is for a couple of reasons.  For one, builtins may all come in
> @@ -1400,6 +1421,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode,
>                 var->members[i]->interface_type =
>                    interface_type->members[i]->type;
>                 var->members[i]->data.mode = nir_mode;
> +               var->members[i]->data.patch = var->patch;
>              }
>           } else {
>              var->var = rzalloc(b->shader, nir_variable);
> @@ -1407,6 +1429,7 @@ vtn_handle_variables(struct vtn_builder *b, SpvOp opcode,
>              var->var->type = var->type->type;
>              var->var->interface_type = interface_type->type;
>              var->var->data.mode = nir_mode;
> +            var->var->data.patch = var->patch;
>           }
>  
>           /* For inputs and outputs, we need to grab locations and builtin
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170111/d4e69262/attachment.sig>