[Mesa-dev] [PATCH] spirv/nir: adjust location assignment for the case of arrays of blocks

[Timothy Arceri]

On 13/12/18 11:11 pm, Alejandro Piñeiro wrote:
> This is needed due how the types get rearranged after the struct
> splitting.
> 
> So for example, this array of blocks:
> 
>    layout(location = 0) out block {
>      vec4 v;
>      vec3 v2;
>    } x[2];
> 
> Would be splitted on two nir variables with the following types:
>    * vec4 v[2]
>    * vec3 v2[2]
> 
> So we need to take into account the length of the array to avoid
> locations overlaps one with the other.
> ---
> 
> 
> Hi Jason,
> 
> again, sending in advance patches, just in case you are working on the
> same.
> 
> I was able to fix the location overlapping without all those crazy
> ideas about lowering array of blocks into individual blocks, by just
> adjusting the locations as this patch shows.
> 
> FWIW, the resulting locations are equivalent to those that we get with
> GLSL IR, that results on a similar splitting.
> 
> With this change I got the following working:
>     * SPIR-V simple arrays of blocks input/outputs
> 
>     * The arrays of blocks inputs/outputs + interpolator qualifiers
>       test I mentioned to you last week [1] when run its SPIR-V
>       equivalent.
> 
>     * SPIR-V xfb tests using arrays of blocks, where the xfb offset are
>       assigned to all block members.
> 
>     * SPIR-V xfb tests using arrays of blocks, where the xfb offset is
>       assigned to just one member, so just that member is captured,
>       although as many times as the array length (yes! afaiu by spec
>       that needs to work)
> 
> So now, the only pending thing is a cleanup and send the series to
> review. Specifically, I think that this series can be put on top of
> current master instead of the arb_gl_spirv. Will try that and send a
> final series this week or early next week.
> 
> BR
> 
> 
> [1] https://github.com/Igalia/piglit/blob/master/tests/spec/glsl-1.50/execution/interface-block-interpolation-array.shader_test
> 
> 
> 
>   src/compiler/spirv/vtn_variables.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/src/compiler/spirv/vtn_variables.c b/src/compiler/spirv/vtn_variables.c
> index a8f2fdfa534..87386cee42f 100644
> --- a/src/compiler/spirv/vtn_variables.c
> +++ b/src/compiler/spirv/vtn_variables.c
> @@ -1672,6 +1672,14 @@ add_missing_member_locations(struct vtn_variable *var,
>         glsl_get_length(glsl_without_array(var->type->type));
>      int location = var->base_location;
>   
> +   /* To know if it is a interface block we can't ask directly for
> +    * var->type->block because on the case of arrays of blocks, block is set
> +    * on the array_element.
> +    */
> +   bool is_array_block = var->var->interface_type != NULL &&
> +      glsl_type_is_array(var->type->type);
> +   int adjustment = is_array_block ? glsl_get_length(var->type->type) : 1;
> +

I think you probably want to use glsl_get_aoa_size() here instead of 
glsl_get_length() ?

>      for (unsigned i = 0; i < length; i++) {
>         /* From the Vulkan spec:
>          *
> @@ -1702,8 +1710,12 @@ add_missing_member_locations(struct vtn_variable *var,
>         const struct glsl_type *member_type =
>            glsl_get_struct_field(glsl_without_array(var->type->type), i);
>   
> +      /* For arrays of interface blocks we can't just add the attribute slots
> +       * of a member type due how the splitting would rearrange the types, so
> +       * we need to adjust for the array length in that case.
> +       */
>         location +=
> -         glsl_count_attribute_slots(member_type, is_vertex_input);
> +         glsl_count_attribute_slots(member_type, is_vertex_input) * adjustment;
>      }
>   }
>   
>