[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 14:09, Timothy Arceri wrote:
> On 19/9/18 7:59 pm, Alejandro Piñeiro wrote:
>> 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.
>
> I think you mean getting a single element of an array?

Perhaps.

>
>> 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.
>
> The code for glsl_get_array_element() is:
>
> const glsl_type *
> glsl_get_array_element(const glsl_type* type)
> {
>    if (type->is_matrix())
>       return type->column_type();
>    else if (type->is_vector())
>       return type->get_scalar_type();
>    return type->fields.array;
> }
>
> I was thinking you might end up stripping away the vec and matrix type
> but as you have glsl_type_is_array(type) in the if condition it is fine.

Ok.

>
>>
>> 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?
>
> Apologizes I pasted the wrong thing I meant:
>
>   type = glsl_without_array(type);

Ok, later today, or tomorrow, I will try with this one.

>
> However looking more closely at the code and the spec I see that's not
> what you want.
>
> However I'm starting to think the quote from the spec and this code
> have nothing to do with each other.

Yes, probably the spec quote is not the best one. We know what the
behaviour should be (based on comments at the GLSL linker, how
uniformstorage is populated on GLSL, and the program interface queries),
so I thought that was the spec quote justifying it. Now that you mention
it, and re-reading, it is true that's not perhaps the best quote
justifying it.

>
> The spec is taking about top level block member arrays. For example:
>
> layout (std140, binding = 5, row_major) uniform ComponentsBlock
>  {
>     float c1[3];
> } components;
>
> However neither of the examples you gave have top level arrays:
>
> layout (std140, binding = 5, row_major) uniform ComponentsBlock
>  {
>     float c1;
>     vec2 c2;
>     S s;
> } components[2];
>
> My guess is you are using this to remove the array from the block
> itself which is applied when the block is split into separate vars.
> However the logic in this patch will fail when you have arrays of
> arrays for example:

Thanks for mentioning AOA. I have just sent an email replying the first
patch of the series. AOA is not supported with this series. And the
first step is adding the support on the spirv to nir pass, so right now
an aoa of ubos test will not run this code.

>
> layout (std140, binding = 5, row_major) uniform ComponentsBlock
>  {
>     float c1;
>     vec2 c2;
>     S s;
> } components[2][2];
>
> Or even something like:
>
> layout (std140, binding = 5, row_major) uniform ComponentsBlock
>  {
>     float c1[3];
>     vec2 c2[3];
>     S s[3];
> } components[2];
>
> It's late here and I've only skimmed over other patches in the series
> so if I'm missing something please correct me.

See my previous comment. I forgot to mention that AOA of ubos/ssbos are
not supported yet. Sorry for that.

>
>>
>>>
>>>> +         }
>>>> +
>>>>             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);
>>>>
>>>
>>
>