[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
Wed Sep 23 17:04:56 PDT 2015
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.
>
> 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