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