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

[Ilia Mirkin]

On Wed, Jan 6, 2016 at 4:32 PM, Timothy Arceri
<timothy.arceri at collabora.com> wrote:
> 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?

Oh I see what's going on now. I took a much more careful look at the
surrounding logic and I think switching expl_location to be init to 0
is fine -- if it's set on the layout it'll be initialized, otherwise
it will never be used. Basically "expl_location" is "what is the
current location that we should assign the next variable to when
there's no explicit location listed on the var, but there is one on
the block".

So actually as originally sent, your patch is

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>