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

[apinheiro]

On 14/12/18 0:54, Timothy Arceri wrote:
>
>
> 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() ?


Ok, thanks for the suggestion, I will try it, and add some extra tests
to verify it.

Having said so, after cleaning this RFC series, I got some regressions
with tessellation cts vulkan tests with Intel CI, starting from this
commit. So unfourtunately, this is still WIP, and probably the final
patch will be different.

Again, thanks for the hint.


>
>>      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;
>>      }
>>   }
>>  
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pEpkey.asc
Type: application/pgp-keys
Size: 1546 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181214/756f4249/attachment-0001.key>