[Mesa-dev] [PATCH v5 70/70] glsl: Mark as active all elements of shared/std140 block arrays
Timothy Arceri
t_arceri at yahoo.com.au
Thu Sep 24 10:01:27 PDT 2015
On 25 September 2015 12:54:38 am AEST, "Samuel Iglesias Gonsálvez" <siglesias at igalia.com> wrote:
>
>
>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.
After a second look I agree, this does need to change. Thanks for double checking.
>
>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
>>>
>>>
>>
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
More information about the mesa-dev
mailing list