On Wed, Jan 6, 2016 at 4:32 PM, Timothy Arceri <timothy.arc...@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.arc...@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 <imir...@alum.mit.edu> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev