[Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables

Iago Toral itoral at igalia.com
Fri Oct 20 05:54:00 UTC 2017


On Fri, 2017-10-20 at 13:18 +1100, Timothy Arceri wrote:
> 
> On 20/10/17 03:31, Iago Toral Quiroga wrote:
> > The existing code was checking the whole interface variable rather
> > than its members, which is not what we want: we want to check
> > aliasing for each member in the interface variable.
> > 
> > Surprisingly, there are piglit tests that verify this and were
> > passing due to a bug in the existing code: when we were computing
> > the last component used by an interface variable we would use
> > the 'vector' path and multiply by vector_elements, which is 0 for
> > interface variables. This made the loop that checks for aliasing
> > be a no-op and not add the interface variable to the list of
> > outputs
> > so then we would fail to link when we did not see a matching output
> > for the same input in the next stage. Since the tests expect a
> > linker error to happen, they would pass, but not for the right
> > reason.
> > 
> > Unfortunately, the current implementation uses ir_variable
> > instances
> > to keep track of explicit locations. Since we don't have
> > ir_variables instances for individual interface members, we need
> > to have a custom struct with the data we need. This struct has
> > the ir_variable (which for interface members is the whole
> > interface variable), plus the data that we need to validate for
> > each aliased location, for now only the base type, which for
> > interface members we will take from the appropriate field inside
> > the interface variable.
> > 
> > Later patches will expand this custom struct so we can also check
> > other requirements for location aliasing, specifically that
> > we have matching interpolation and auxiliary storage, that once
> > again, we will take from the appropriate field members for the
> > interface variables.
> > 
> > Fixes:
> > KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations
> > 
> > Fixes:
> > tests/spec/arb_separate_shader_objects/execution/layout-location-
> > named-block.shader_test -auto -fbo
> > 
> > Fixes (these were passing before but for incorrect reasons):
> > tests/spec/arb_enhanced_layouts/linker/block-member-
> > locations/named-block-member-location-overlap.shader_test
> > tests/spec/arb_enhanced_layouts/linker/block-member-
> > locations/named-block-member-mixed-order-overlap.shader_test
> > 
> > ---
> >   src/compiler/glsl/link_varyings.cpp | 45
> > +++++++++++++++++++++++++++----------
> >   1 file changed, 33 insertions(+), 12 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 687bd50e52..5dc2704321 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable
> > *var, gl_shader_stage stage)
> >      return var->data.location - location_start;
> >   }
> >   
> > +struct explicit_location_info {
> > +   ir_variable *var;
> > +   unsigned base_type;
> > +};
> > +
> >   static bool
> > -check_location_aliasing(ir_variable *explicit_locations[][4],
> > +check_location_aliasing(struct explicit_location_info
> > explicit_locations[][4],
> >                           ir_variable *var,
> >                           unsigned location,
> >                           unsigned component,
> > @@ -427,7 +432,7 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> >      while (location < location_limit) {
> >         unsigned i = component;
> >         while (i < last_comp) {
> > -         if (explicit_locations[location][i] != NULL) {
> > +         if (explicit_locations[location][i].var != NULL) {
> >               linker_error(prog,
> >                            "%s shader has multiple outputs
> > explicitly "
> >                            "assigned to location %d and component
> > %d\n",
> > @@ -439,9 +444,9 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> >            /* Make sure all component at this location have the
> > same type.
> >             */
> >            for (unsigned j = 0; j < 4; j++) {
> > -            if (explicit_locations[location][j] &&
> > -                (explicit_locations[location][j]->type-
> > >without_array()
> > -                 ->base_type != type->without_array()->base_type)) 
> > {
> > +            if (explicit_locations[location][j].var &&
> > +                explicit_locations[location][j].base_type !=
> > +                  type->without_array()->base_type) {
> >                  linker_error(prog,
> >                               "Varyings sharing the same location
> > must "
> >                               "have the same underlying numerical
> > type. "
> > @@ -450,7 +455,9 @@ check_location_aliasing(ir_variable
> > *explicit_locations[][4],
> >               }
> >            }
> >   
> > -         explicit_locations[location][i] = var;
> > +         explicit_locations[location][i].var = var;
> > +         explicit_locations[location][i].base_type =
> > +            type->without_array()->base_type;
> >            i++;
> >   
> >            /* We need to do some special handling for doubles as
> > dvec3 and
> > @@ -482,8 +489,8 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> >                                    gl_linked_shader *consumer)
> >   {
> >      glsl_symbol_table parameters;
> > -   ir_variable *explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
> > -      { {NULL, NULL} };
> > +   struct explicit_location_info
> > explicit_locations[MAX_VARYINGS_INCL_PATCH][4] =
> > +      { 0 };
> 
> I think this need to be something like:
> 
>   {{{ 0 }, { 0 }}};
> 
> ???

I didn't do that because it doesn't seem to be necessary after
inspection.

Since then, I have also seen this:
https://stackoverflow.com/questions/15520880/initializing-entire-2d-arr
ay-with-one-value

which explains how this works and pretty much comes to say that
initializing to { 0 } has the same effect.

> 
> >   
> >      /* Find all shader outputs in the "producer" stage.
> >       */
> > @@ -514,9 +521,23 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> >               return;
> >            }
> >   
> > -         if (!check_location_aliasing(explicit_locations, var,
> > idx,
> > -                                      var->data.location_frac,
> > slot_limit,
> > -                                      type, prog, producer-
> > >Stage)) {
> > +         if (type->without_array()->is_interface()) {
> > +            for (unsigned i = 0; i < type->without_array()-
> > >length; i++) {
> > +               const glsl_type *field_type = type-
> > >fields.structure[i].type;
> > +               unsigned field_location =
> > +                     type->fields.structure[i].location -
> > VARYING_SLOT_VAR0;
> > +               if (!check_location_aliasing(explicit_locations,
> > var,
> > +                                            field_location,
> > +                                            0, field_location + 1,
> 
> The spec says interface members can have component qualifiers,
> however 
> it seems this is an existing bug.

Yes, I mentioned that in the cover letter.

> With the initialization of explicit_locations fixed above. 1-2 are:

I think it is not necessary by my reply above, but I am okay with
changing it if you insist, just let me know if that is the case :)

> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
> 
> 
> > +                                            field_type, prog,
> > +                                            producer->Stage)) {
> > +                  return;
> > +               }
> > +            }
> > +         } else if (!check_location_aliasing(explicit_locations,
> > var,
> > +                                             idx, var-
> > >data.location_frac,
> > +                                             slot_limit, type,
> > prog,
> > +                                             producer->Stage)) {
> >               return;
> >            }
> >         }
> > @@ -574,7 +595,7 @@ cross_validate_outputs_to_inputs(struct
> > gl_context *ctx,
> >               unsigned slot_limit = idx + num_elements;
> >   
> >               while (idx < slot_limit) {
> > -               output = explicit_locations[idx][input-
> > >data.location_frac];
> > +               output = explicit_locations[idx][input-
> > >data.location_frac].var;
> >   
> >                  if (output == NULL ||
> >                      input->data.location != output->data.location) 
> > {
> > 
> 
> 


More information about the mesa-dev mailing list