[Mesa-dev] [PATCH v2 10/8] glsl/linker: Fix type checks for location aliasing

[Iago Toral]

On Wed, 2017-10-25 at 08:30 -0400, Ilia Mirkin wrote:
> On Wed, Oct 25, 2017 at 5:15 AM, Iago Toral Quiroga <itoral at igalia.co
> m> wrote:
> > From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers,
> > Page 68,
> > (Location aliasing):
> > 
> >    "Further, when location aliasing, the aliases sharing the
> > location
> >     must have the same underlying numerical type  (floating-point
> > or
> >     integer)."
> > 
> > The current implementation is too strict, since it checks that the
> > the base types are an exact match instead.
> > ---
> >  src/compiler/glsl/link_varyings.cpp | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index 8767f00976..26d3157c1e 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -405,13 +405,28 @@ compute_variable_location_slot(ir_variable
> > *var, gl_shader_stage stage)
> > 
> >  struct explicit_location_info {
> >     ir_variable *var;
> > -   unsigned base_type;
> > +   unsigned numerical_type;
> >     unsigned interpolation;
> >     bool centroid;
> >     bool sample;
> >     bool patch;
> >  };
> > 
> > +static inline unsigned
> > +get_numerical_type(const glsl_type *type)
> > +{
> > +   /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout
> > Qualifiers, Page 68,
> > +    * (Location aliasing):
> > +    *
> > +    *    "Further, when location aliasing, the aliases sharing the
> > location
> > +    *     must have the same underlying numerical type  (floating-
> > point or
> > +    *     integer)
> 
> This is open to interpretation. In my version, I also disallowed
> mixing doubles with floats, and int64's with ints (or floats or
> doubles).
> 
> I don't object to this interpretation, but perhaps you can raise a
> question with Khronos regarding what exactly is incompatible.

Sure, I will try to clarify this with them.

> Either way, this patch is Reviewed-by: Ilia Mirkin
> <imirkin at alum.mit.edu>

Thanks!

> > +    */
> > +   if (type->is_float() || type->is_double())
> > +      return GLSL_TYPE_FLOAT;
> > +   return GLSL_TYPE_INT;
> > +}
> > +
> >  static bool
> >  check_location_aliasing(struct explicit_location_info
> > explicit_locations[][4],
> >                          ir_variable *var,
> > @@ -456,7 +471,8 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> >                 /* For all other used components we need to have
> > matching
> >                  * types, interpolation and auxiliary storage
> >                  */
> > -               if (info->base_type != type->without_array()-
> > >base_type) {
> > +               if (info->numerical_type !=
> > +                   get_numerical_type(type->without_array())) {
> >                    linker_error(prog,
> >                                 "Varyings sharing the same location
> > must "
> >                                 "have the same underlying numerical
> > type. "
> > @@ -486,7 +502,7 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> >              }
> >           } else if (comp >= component && comp < last_comp) {
> >              info->var = var;
> > -            info->base_type = type->without_array()->base_type;
> > +            info->numerical_type = get_numerical_type(type-
> > >without_array());
> >              info->interpolation = interpolation;
> >              info->centroid = centroid;
> >              info->sample = sample;
> > --
> > 2.11.0
> > 
> 
>