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

[Timothy Arceri]

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?

> 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.

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

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.

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:

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.

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