[Mesa-dev] [PATCH] glsl: fix varying slot allocation for blocks and structs with explicit locations

[Timothy Arceri]

On Wed, 2016-01-06 at 09:46 -0500, Ilia Mirkin wrote:
> On Wed, Jan 6, 2016 at 4:32 AM, Timothy Arceri
> <timothy.arceri at collabora.com> wrote:
> > Previously each member was being counted as using a single slot,
> > count_attribute_slots() fixes the count for array and struct
> > members.
> > 
> > Also don't assign a negitive to the unsigned expl_location
> > variable.
> > ---
> > 
> >  Fixes these new piglit tests:
> >    http://patchwork.freedesktop.org/patch/69531/
> > 
> >  src/glsl/ast_to_hir.cpp | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > index 0197cdc..50d5e22 100644
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
> > @@ -6408,12 +6408,13 @@
> > ast_process_struct_or_iface_block_members(exec_list *instructions,
> >              if (process_qualifier_constant(state, &loc,
> > "location",
> >                                             qual->location,
> > &qual_location)) {
> >                 fields[i].location = VARYING_SLOT_VAR0 +
> > qual_location;
> > -               expl_location = fields[i].location + 1;
> > +               expl_location = fields[i].location +
> > +                  fields[i].type->count_attribute_slots(false);
> >              }
> >           } else {
> >              if (layout && layout->flags.q.explicit_location) {
> >                 fields[i].location = expl_location;
> > -               expl_location = expl_location + 1;
> > +               expl_location += fields[i].type
> > ->count_attribute_slots(false);
> >              } else {
> >                 fields[i].location = -1;
> >              }
> > @@ -6570,7 +6571,7 @@ ast_struct_specifier::hir(exec_list
> > *instructions,
> > 
> >     state->struct_specifier_depth++;
> > 
> > -   unsigned expl_location = -1;
> > +   unsigned expl_location = 0;
> >     if (layout && layout->flags.q.explicit_location) {
> >        if (!process_qualifier_constant(state, &loc, "location",
> >                                        layout->location,
> > &expl_location)) {
> > @@ -6763,7 +6764,7 @@ ast_interface_block::hir(exec_list
> > *instructions,
> >        return NULL;
> >     }
> > 
> > -   unsigned expl_location = -1;
> > +   unsigned expl_location = 0;
> 
> There are a number of places that check for location != -1 as a
> sanity
> check... won't this defeat that?

No because we only use expl_location when the explicit location flag is
set and if there is an error we don't copy the value from
expl_location.

I believe I initialised it to stop gcc complaining although I just
tried removing this and it no longer complains so I guess I can just
remove the initialisation altogether.

Are you happy with the change otherwise?

> 
> >     if (layout.flags.q.explicit_location) {
> >        if (!process_qualifier_constant(state, &loc, "location",
> >                                        layout.location,
> > &expl_location)) {
> > --
> > 2.4.3
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev