[Mesa-dev] [PATCH 07/28] spirv/nir: don't set interface_type if it is not a struct

[Timothy Arceri]

On 22/10/18 11:24 pm, Alejandro Piñeiro wrote:
> vnt_variables uses interface_type on several use cases, but on nir
> variable it is more limited. From nir.h:
> 
>     /**
>       * For variables that are in an interface block or are an instance of an
>       * interface block, this is the \c GLSL_TYPE_INTERFACE type for that block.
>       *
>       * \sa ir_variable::location
>       */
> 
> But interface blocks expects the type to be an struct, so those cases
> should not be filled. For example, glsl checks if a variable is in an
> uniform block if it is an uniform and has an interface type.
> 
> One example of why this is needed: gl_PatchVerticesIn is lowered to an
> uniform. Without this change, it would include a interface_type. Then,
> we would try to initialize the uniform block, and find that it doesn't
> have any component.
> ---
>   src/compiler/spirv/vtn_variables.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c
> index 957ef0610b7..9a4ddeaa822 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1818,6 +1818,8 @@ vtn_create_variable(struct vtn_builder *b, struct vtn_value *val,
>               var->var->members[i].mode = nir_mode;
>               var->var->members[i].patch = var->patch;
>            }
> +      } else {
> +         var->var->interface_type = NULL;
>         }
>   
>         /* For inputs and outputs, we need to grab locations and builtin
> 

Please don't do these bandaid style changes, they make the code hard to 
follow and error prone. We should instead change the code so that 
interface_type is never set unless we are processing a struct:


       var->var = rzalloc(b->shader, nir_variable);
       var->var->name = ralloc_strdup(var->var, val->name);
       var->var->type = var->type->type;
       var->var->data.mode = nir_mode;
       var->var->data.patch = var->patch;

       /* For inputs and outputs, we immediately split structures.  This
        * is for a couple of reasons.  For one, builtins may all come in
        * a struct and we really want those split out into separate
        * variables.  For another, interpolation qualifiers can be
        * applied to members of the top-level struct ane we need to be
        * able to preserve that information.
        */

       struct vtn_type *interface_type = var->type;
       if (is_per_vertex_inout(var, b->shader->info.stage)) {
          /* In Geometry shaders (and some tessellation), inputs come
           * in per-vertex arrays.  However, some builtins come in
           * non-per-vertex, hence the need for the is_array check.  In
           * any case, there are no non-builtin arrays allowed so this
           * check should be sufficient.
           */
          interface_type = var->type->array_element;
       }

       if (glsl_type_is_struct(interface_type->type)) {
          var->var->interface_type = interface_type->type;

          /* It's a struct.  Set it up as per-member. */
          var->var->num_members = glsl_get_length(interface_type->type);
          var->var->members = rzalloc_array(var->var, struct 
nir_variable_data,
                                            var->var->num_members);

          for (unsigned i = 0; i < var->var->num_members; i++) {
             var->var->members[i].mode = nir_mode;
             var->var->members[i].patch = var->patch;
          }
       }

With that change this is:

Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>