[Mesa-dev] [PATCH v5 70/70] glsl: Mark as active all elements of shared/std140 block arrays

[Antía Puentes]

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?

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/095070.
> > 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_dereference_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