[Mesa-dev] [PATCH v2 9/8] glsl/linker: refactor check_location_aliasing

[Iago Toral]

On Wed, 2017-10-25 at 08:35 -0400, Ilia Mirkin wrote:
> Not your fault, but this diff is hard to read. On the bright side, 20
> lines removed... Can you confirm that
> 
> layout(location = 1, component = 2) int foo;
> layout(location = 0) dvec3 bar;
> 
> fails due to the int vs double conflicts? (I'm specifically
> interested
> in the case where the int is specified first and dvec3 is specified
> second, since it'll be up to the processing of the dvec3 to determine
> the conflict on the second slot.)
> 
> Assuming that all checks out,

Yeah, I have checked this (and a few other variations) and everything
seems to work as expected.

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


Thanks to you and Timothy for all the reviews!

Iago
> 
> On Wed, Oct 25, 2017 at 5:15 AM, Iago Toral Quiroga <itoral at igalia.co
> m> wrote:
> > Mostly, this merges the type checks with all the other checks so
> > we only have a single loop for this.
> > ---
> >  src/compiler/glsl/link_varyings.cpp | 110 +++++++++++++++---------
> > ------------
> >  1 file changed, 46 insertions(+), 64 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index e80736fb38..8767f00976 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -438,92 +438,74 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> >     }
> > 
> >     while (location < location_limit) {
> > -      unsigned i = component;
> > -
> > -      /* If there are other outputs assigned to the same location
> > -       * they must have the same interpolation
> > -       */
> >        unsigned comp = 0;
> >        while (comp < 4) {
> > -         /* Skip the components used by this output, we only care
> > about
> > -         * other outputs in the same location
> > -          */
> > -         if (comp == i) {
> > -            comp = last_comp;
> > -            continue;
> > -         }
> > -
> >           struct explicit_location_info *info =
> >              &explicit_locations[location][comp];
> > 
> >           if (info->var) {
> > -            if (info->interpolation != interpolation) {
> > +            /* Component aliasing is not alloed */
> > +            if (comp >= component && comp < last_comp) {
> >                 linker_error(prog,
> > -                            "%s shader has multiple outputs at
> > explicit "
> > -                            "location %u with different
> > interpolation "
> > -                            "settings\n",
> > -                            _mesa_shader_stage_to_string(stage),
> > location);
> > +                            "%s shader has multiple outputs
> > explicitly "
> > +                            "assigned to location %d and component
> > %d\n",
> > +                            _mesa_shader_stage_to_string(stage),
> > +                            location, component);
> >                 return false;
> > -            }
> > -
> > -            if (info->centroid != centroid ||
> > -                info->sample != sample ||
> > -                info->patch != patch) {
> > -               linker_error(prog,
> > -                            "%s shader has multiple outputs at
> > explicit "
> > -                            "location %u with different aux
> > storage\n",
> > -                            _mesa_shader_stage_to_string(stage),
> > location);
> > -               return false;
> > -            }
> > -         }
> > -
> > -         comp++;
> > -      }
> > +            } else {
> > +               /* For all other used components we need to have
> > matching
> > +                * types, interpolation and auxiliary storage
> > +                */
> > +               if (info->base_type != type->without_array()-
> > >base_type) {
> > +                  linker_error(prog,
> > +                               "Varyings sharing the same location
> > must "
> > +                               "have the same underlying numerical
> > type. "
> > +                               "Location %u component %u\n",
> > +                               location, component);
> > +                  return false;
> > +               }
> > 
> > -      /* Component aliasing is not allowed */
> > -      while (i < last_comp) {
> > -         if (explicit_locations[location][i].var != NULL) {
> > -            linker_error(prog,
> > -                         "%s shader has multiple outputs
> > explicitly "
> > -                         "assigned to location %d and component
> > %d\n",
> > -                         _mesa_shader_stage_to_string(stage),
> > -                         location, component);
> > -            return false;
> > -         }
> > +               if (info->interpolation != interpolation) {
> > +                  linker_error(prog,
> > +                               "%s shader has multiple outputs at
> > explicit "
> > +                               "location %u with different
> > interpolation "
> > +                               "settings\n",
> > +                               _mesa_shader_stage_to_string(stage)
> > , location);
> > +                  return false;
> > +               }
> > 
> > -         /* Make sure all component at this location have the same
> > type.
> > -          */
> > -         for (unsigned j = 0; j < 4; j++) {
> > -            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. "
> > -                            "Location %u component %u\n",
> > location, component);
> > -               return false;
> > +               if (info->centroid != centroid ||
> > +                   info->sample != sample ||
> > +                   info->patch != patch) {
> > +                  linker_error(prog,
> > +                               "%s shader has multiple outputs at
> > explicit "
> > +                               "location %u with different aux
> > storage\n",
> > +                               _mesa_shader_stage_to_string(stage)
> > , location);
> > +                  return false;
> > +               }
> >              }
> > +         } else if (comp >= component && comp < last_comp) {
> > +            info->var = var;
> > +            info->base_type = type->without_array()->base_type;
> > +            info->interpolation = interpolation;
> > +            info->centroid = centroid;
> > +            info->sample = sample;
> > +            info->patch = patch;
> >           }
> > 
> > -         explicit_locations[location][i].var = var;
> > -         explicit_locations[location][i].base_type =
> > -            type->without_array()->base_type;
> > -         explicit_locations[location][i].interpolation =
> > interpolation;
> > -         explicit_locations[location][i].centroid = centroid;
> > -         explicit_locations[location][i].sample = sample;
> > -         explicit_locations[location][i].patch = patch;
> > -         i++;
> > +         comp++;
> > 
> >           /* We need to do some special handling for doubles as
> > dvec3 and
> >            * dvec4 consume two consecutive locations. We don't need
> > to
> >            * worry about components beginning at anything other
> > than 0 as
> >            * the spec does not allow this for dvec3 and dvec4.
> >            */
> > -         if (i == 4 && last_comp > 4) {
> > +         if (comp == 4 && last_comp > 4) {
> >              last_comp = last_comp - 4;
> >              /* Bump location index and reset the component index
> > */
> >              location++;
> > -            i = 0;
> > +            comp = 0;
> > +            component = 0;
> >           }
> >        }
> > 
> > --
> > 2.11.0
> > 
> 
>