[Mesa-dev] [PATCH v5 70/70] glsl: Mark as active all elements of shared/std140 block arrays
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Thu Sep 24 07:54:38 PDT 2015
On 24/09/15 02:04, Timothy Arceri wrote:
> On Wed, 2015-09-23 at 17:02 +0200, Antía Puentes wrote:
>> Hi! Timothy,
>> thanks for your review.
>>
>> Seeing your patch in:
>> http://lists.freedesktop.org/archives/mesa-dev/2015-September/095070.
>> html
>>
>> The condition in my patch:
>> (var->type->interface_packing != GLSL_INTERFACE_PACKING_PACKED)
>> should also be changed to:
>> (var->get_interface_type()->interface_packing !=
>> GLSL_INTERFACE_PACKING_PACKED)
>>
>> Right?
>
> You don't really need to do that because var is already the variable
> not the array and we know its the interface not an interface member
> because of the conditions above your change.
>
'var->type' refers to the array of interface block here but the
interface packing information is hold inside the interface block type.
Because of that, we need to use
var->get_interface_type()->interface_packing to get the valid
information for the condition, so we don't do the wrong thing.
I even ran a GDB session locally with one of my programs to be sure
before replying :-)
If you agree, we are going to do that change before pushing the patch to
master.
Sam
[0]
http://lists.freedesktop.org/archives/mesa-dev/2015-September/094009.html
>
>>
>> On mié, 2015-09-23 at 23:44 +1000, Timothy Arceri wrote:
>>> On Wed, 2015-09-23 at 12:25 +0200, Samuel Iglesias Gonsálvez wrote:
>>>>
>>>> On 21/09/15 13:11, Samuel Iglesias Gonsálvez wrote:
>>>>> On 21/09/15 09:41, Tapani Pälli wrote:
>>>>>> Seems like a nice fix, takes ES3 CTS failures from 116 to 64
>>>>>> on
>>>>>> Haswell
>>>>>> (most failing tests are with ubos).
>>>>>>
>>>>>> Tested-by: Tapani Pälli <tapani.palli at intel.com>
>>>>>>
>>>>>
>>>>> OK thanks!
>>>>>
>>>>>> This is individual patch not related to just SSBOs, maybe
>>>>>> this
>>>>>> could be
>>>>>> pushed separately from the rest?
>>>>>>
>>>>>
>>>>> Yes, this patch can be pushed separately from the rest of
>>>>> patches
>>>>> of the
>>>>> series: we just need an R-b, as usual.
>>>>>
>>>>> We are going to discuss the proper solution with Timothy [0],
>>>>> as he
>>>>> found that we are not covering one case.
>>>>>
>>>>
>>>> Timothy has sent a patch fixing the packed case [0] and he
>>>> developed
>>>> a
>>>> piglit test for it [1].
>>>>
>>>> We are going to wait for an R-b of Antía's patch before pushing
>>>> it.
>>>
>>> I sent a reply to this same email saying almost the same thing but
>>> it
>>> seems to have gone missing.
>>>
>>> Anyway I also sent my r-b, this patch looks good to me.
>>>
>>> Reviewed-by: Timothy Arceri <t_arceri at yahoo.com.au>
>>>
>>>
>>>>
>>>> Sam
>>>>
>>>> [0]
>>>> http://lists.freedesktop.org/archives/mesa-dev/2015-September/095
>>>> 070.
>>>> html
>>>> [1]
>>>> http://lists.freedesktop.org/archives/piglit/2015-September/01724
>>>> 7.html
>>>>
>>>>> Sam
>>>>>
>>>>> [0] https://bugs.freedesktop.org/show_bug.cgi?id=83508
>>>>>
>>>>>
>>>>>> // Tapani
>>>>>>
>>>>>> On 09/10/2015 04:36 PM, Iago Toral Quiroga wrote:
>>>>>>> From: Antia Puentes <apuentes at igalia.com>
>>>>>>>
>>>>>>> Commit 1ca25ab (glsl: Do not eliminate 'shared' or 'std140'
>>>>>>> blocks
>>>>>>> or block members) considered as active 'shared' and
>>>>>>> 'std140'
>>>>>>> uniform
>>>>>>> blocks and uniform block arrays, but did not include the
>>>>>>> block
>>>>>>> array
>>>>>>> elements. Because of that, it was possible to have an
>>>>>>> active
>>>>>>> uniform
>>>>>>> block array without any elements marked as used, making the
>>>>>>> assertion
>>>>>>> ((b->num_array_elements > 0) == b->type->is_array())
>>>>>>> in link_uniform_blocks() fail.
>>>>>>>
>>>>>>> Fixes the following 5 dEQP tests:
>>>>>>>
>>>>>>> * dEQP
>>>>>>> -GLES3.functional.ubo.random.nested_structs_instance_arrays
>>>>>>> .18
>>>>>>> * dEQP
>>>>>>> -GLES3.functional.ubo.random.nested_structs_instance_arrays
>>>>>>> .24
>>>>>>> *
>>>>>>> dEQP
>>>>>>> -GLES3.functional.ubo.random.nested_structs_arrays_instance
>>>>>>> _arr
>>>>>>> ays.19
>>>>>>> * dEQP
>>>>>>> -GLES3.functional.ubo.random.all_per_block_buffers.49
>>>>>>> * dEQP-GLES3.functional.ubo.random.all_shared_buffer.36
>>>>>>>
>>>>>>> Fixes bugzilla:
>>>>>>> https://bugs.freedesktop.org/show_bug.cgi?id=83508
>>>>>>> ---
>>>>>>> src/glsl/link_uniform_block_active_visitor.cpp | 23
>>>>>>> +++++++++++++++++++++++
>>>>>>> 1 file changed, 23 insertions(+)
>>>>>>>
>>>>>>> diff --git a/src/glsl/link_uniform_block_active_visitor.cpp
>>>>>>> b/src/glsl/link_uniform_block_active_visitor.cpp
>>>>>>> index 5102947..fbe79de 100644
>>>>>>> --- a/src/glsl/link_uniform_block_active_visitor.cpp
>>>>>>> +++ b/src/glsl/link_uniform_block_active_visitor.cpp
>>>>>>> @@ -106,6 +106,22 @@
>>>>>>> link_uniform_block_active_visitor::visit(ir_variable *var)
>>>>>>> assert(b->num_array_elements == 0);
>>>>>>> assert(b->array_elements == NULL);
>>>>>>> assert(b->type != NULL);
>>>>>>> + assert(!b->type->is_array() || b->has_instance_name);
>>>>>>> +
>>>>>>> + /* For uniform block arrays declared with a shared or
>>>>>>> std140 layout
>>>>>>> + * qualifier, mark all its instances as used.
>>>>>>> + */
>>>>>>> + if (b->type->is_array() && b->type->length > 0) {
>>>>>>> + b->num_array_elements = b->type->length;
>>>>>>> + b->array_elements = reralloc(this->mem_ctx,
>>>>>>> + b->array_elements,
>>>>>>> + unsigned,
>>>>>>> + b->num_array_elements);
>>>>>>> +
>>>>>>> + for (unsigned i = 0; i < b->num_array_elements; i++)
>>>>>>> {
>>>>>>> + b->array_elements[i] = i;
>>>>>>> + }
>>>>>>> + }
>>>>>>>
>>>>>>> return visit_continue;
>>>>>>> }
>>>>>>> @@ -147,6 +163,13 @@
>>>>>>> link_uniform_block_active_visitor::visit_enter(ir_dereferen
>>>>>>> ce_a
>>>>>>> rray *ir)
>>>>>>> assert((b->num_array_elements == 0) == (b
>>>>>>> ->array_elements
>>>>>>> == NULL));
>>>>>>> assert(b->type != NULL);
>>>>>>>
>>>>>>> + /* If the block array was declared with a shared or
>>>>>>> + * std140 layout qualifier, all its instances have been
>>>>>>> already
>>>>>>> marked
>>>>>>> + * as used in
>>>>>>> link_uniform_block_active_visitor::visit(ir_variable
>>>>>>> *).
>>>>>>> + */
>>>>>>> + if (var->type->interface_packing !=
>>>>>>> GLSL_INTERFACE_PACKING_PACKED)
>>>>>>> + return visit_continue_with_parent;
>>>>>>> +
>>>>>>> ir_constant *c = ir->array_index->as_constant();
>>>>>>>
>>>>>>> if (c) {
>>>>>>>
>>>>>> _______________________________________________
>>>>>> mesa-dev mailing list
>>>>>> mesa-dev at lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>> _______________________________________________
>>>>> mesa-dev mailing list
>>>>> mesa-dev at lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>>
>
More information about the mesa-dev
mailing list