[Mesa-dev] [PATCH 19/26] nir/linker: use only the array element type for array of ssbo/ubo

[Alejandro Piñeiro]

On 19/09/18 07:20, Timothy Arceri wrote:
> On 16/9/18 2:18 am, Alejandro Piñeiro wrote:
>> For this interfaces, the inner members are added only once as uniforms
>> or resources, in opposite to other cases, like a uniform array of
>> structs.
>> ---
>>   src/compiler/glsl/gl_nir_link_uniforms.c | 26
>> ++++++++++++++++++++++++--
>>   1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/gl_nir_link_uniforms.c
>> b/src/compiler/glsl/gl_nir_link_uniforms.c
>> index 00995fb3f76..c692cd0171f 100644
>> --- a/src/compiler/glsl/gl_nir_link_uniforms.c
>> +++ b/src/compiler/glsl/gl_nir_link_uniforms.c
>> @@ -498,11 +498,33 @@ gl_nir_link_uniforms(struct gl_context *ctx,
>>              state.current_var = var;
>>   +         /*
>> +          * From OpenGL 4.6 spec, section 7.3.1.1, "Naming Active
>> Resources":
>> +          *
>> +          *   "* For an active shader storage block member declared
>> as an
>> +          *      array of an aggregate type, an entry will be
>> generated only
>> +          *      for the first array element, regardless of its
>> type. Such
>> +          *      block members are referred to as top-level arrays.
>> If the
>> +          *      block member is an aggregate type, the enumeration
>> rules are
>> +          *      then applied recursively.
>> +          *    * For an active interface block not declared as an
>> array of
>> +          *      block instances, a single entry will be generated,
>> using the
>> +          *      block name from the shader source."
>> +          *
>> +          * So for the UBO and SSBO case, we expand only the array
>> element
>> +          * type.
>> +          */
>> +         const struct glsl_type *type = var->type;
>> +         if (nir_variable_is_in_block(var) &&
>> +             glsl_type_is_array(type)) {
>> +            type = glsl_get_array_element(type);
>
> This also strips away vector and matrix type information while leaving
> arrays for anything but a single dimension array.

Well, but the idea behind this patch is getting a single dimension
array. We are using the type to populate the uniforms, and as mentioned
on the spec quote, for ubo/ssbo and recursively for his entries, it is
done just once, no matter the dimension of the ubo/ssbo array.

Also not sure why do you mean that we would lose matrix and vector type
information. Could you give an example.

FWIW, this patch is needed to get this test working:
https://github.com/Igalia/piglit/blob/arb_gl_spirv-series5-ubo-ssbo-v1/tests/spec/arb_gl_spirv/execution/ubo/array-complex.shader_test

and just in case, I have just writing a similar test, slightly more
complex, but with array members and array of matrices members:

https://github.com/Igalia/piglit/blob/arb_gl_spirv-series5-ubo-ssbo-v1/tests/spec/arb_gl_spirv/execution/ubo/array-complex-2.shader_test

And the uniform and variable count is the same on both GLSL and SPIR-V.

>
> Are you sure you don't want the following instead?
>
> type = glsl_get_array_element(type);

Sorry, but isn't that what we are already doing? Or do you mean that we
should made the call always, without the is_in_block and is_array checks?

>
>> +         }
>> +
>>            struct type_tree_entry *type_tree =
>> -            build_type_tree_for_type(var->type);
>> +            build_type_tree_for_type(type);
>>            state.current_type = type_tree;
>>   -         int res = nir_link_uniform(ctx, prog, sh->Program,
>> shader_type, var->type,
>> +         int res = nir_link_uniform(ctx, prog, sh->Program,
>> shader_type, type,
>>                                       location, &state);
>>              free_type_tree(type_tree);
>>
>