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

[Alejandro Piñeiro]

On 20/09/18 00:22, Timothy Arceri wrote:
>
>
> On 19/9/18 11:10 pm, Alejandro Piñeiro wrote:
>>
>> 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.
>
> As I said below on closer look I don't actually think this is what you
> want.
>
>>
>>>
>>> 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.

Finally was able to check this. Sorry for needing so much time, XDC
about to happen.

>
> This last one is not technically an array of arrays and should be
> handled by this series IMO.

I have updated the test which included a more complex array of ubo, that
includes something similar to the last one. That is working too, both
with get_array_element and without_array. The issue is that although on
the spec quote I used talk about members of the ssbo/block, those
methods were not removing it recursively. And as you say below, what we
just want it to remove the array of the block.

So in the end I think that the problem is with the spec quote I used,
and not with the code itself. Sorry for all the noise. In any case, as I
need to update the patch to include new spec quotes, I will use
glsl_without_array, as it looks more natural for that case.

>
> This patch just doesn't feel right to me. I can understand removing
> the array from the block. But removing the array from the member is
> not what we do in the GLSL linker as far as I can tell.

Yes, you are right. We don't want, and as I said, we were not even
removing, the array from the member.

BR