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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
