[v4,1/6] glsl/linker: location aliasing requires types to have the same width

Submitted by Andres Gomez on Feb. 1, 2019, 6:05 p.m.

Details

Message ID 20190201180600.7327-2-agomez@igalia.com
State Superseded
Headers show
Series "glsl/linker: several arb_enhanced_layouts related fixes" ( rev: 3 2 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Andres Gomez Feb. 1, 2019, 6:05 p.m.
From: Iago Toral Quiroga <itoral@igalia.com>

Regarding location aliasing requirements, the OpenGL spec says:

  "Further, when location aliasing, the aliases sharing the location
   must have the same underlying numerical type  (floating-point or
   integer)."

Khronos has further clarified that this also requires the underlying
types to have the same width, so we can't put a float and a double
in the same location slot for example. Future versions of the spec will
be corrected to make this clear.

This patch amends our implementation to account for this restriction.

In the process of doing this, I also noticed that we would attempt
to check aliasing requirements for record variables (including the test
for the numerical type) which is not allowed, instead, we should be
producing a linker error as soon as we see any attempt to do location
aliasing on non-numerical variables. For the particular case of structs,
we were producing a linker error in this case, but only because we
assumed that struct fields use all components in each location, so
any attempt to alias locations consumed by struct fields would produce
a link error due to component aliasing, which is not accurate of the
actual problem. This patch would make it produce an error for attempting
to alias a non-numerical variable instead, which is always accurate.

v2:
  - Do not assert if we see invalid numerical types. These come
    straight from shader code, so we should produce linker errors if
    shaders attempt to do location aliasing on variables that are not
    numerical such as records.
  - While we are at it, improve error reporting for the case of
    numerical type mismatch to include the shader stage.

v3:
  - Allow location aliasing of images and samplers. If we get these
    it means bindless support is active and they should be handled
    as 64-bit integers (Ilia)
  - Make sure we produce link errors for any non-numerical type
    for which we attempt location aliasing, not just structs.

v4:
  - Rebased with minor fixes (Andres).
  - Added fixing tag to the commit log (Andres).

Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Signed-off-by: Andres Gomez <agomez@igalia.com>
---
 src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
 1 file changed, 46 insertions(+), 18 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
index 3969c0120b3..3f41832ac93 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -424,15 +424,15 @@  compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
 
 struct explicit_location_info {
    ir_variable *var;
-   unsigned numerical_type;
+   int numerical_type;
    unsigned interpolation;
    bool centroid;
    bool sample;
    bool patch;
 };
 
-static inline unsigned
-get_numerical_type(const glsl_type *type)
+static inline int
+get_numerical_sized_type(const glsl_type *type)
 {
    /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
     * (Location aliasing):
@@ -440,10 +440,25 @@  get_numerical_type(const glsl_type *type)
     *    "Further, when location aliasing, the aliases sharing the location
     *     must have the same underlying numerical type  (floating-point or
     *     integer)
+    *
+    * Khronos has further clarified that this also requires the underlying
+    * types to have the same width, so we can't put a float and a double
+    * in the same location slot for example. Future versions of the spec will
+    * be corrected to make this clear.
+    *
+    * Notice that we allow location aliasing for bindless image/samplers too
+    * since these are defined as 64-bit integers.
     */
-   if (type->is_float() || type->is_double())
+   if (type->is_float())
       return GLSL_TYPE_FLOAT;
-   return GLSL_TYPE_INT;
+   else if (type->is_integer())
+      return GLSL_TYPE_INT;
+   else if (type->is_double())
+      return GLSL_TYPE_DOUBLE;
+   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
+      return GLSL_TYPE_INT64;
+
+   return -1; /* Not a numerical type */
 }
 
 static bool
@@ -461,14 +476,17 @@  check_location_aliasing(struct explicit_location_info explicit_locations[][4],
                         gl_shader_stage stage)
 {
    unsigned last_comp;
-   if (type->without_array()->is_record()) {
-      /* The component qualifier can't be used on structs so just treat
-       * all component slots as used.
+   const glsl_type *type_without_array = type->without_array();
+   int numerical_type = get_numerical_sized_type(type_without_array);
+   if (-1 == numerical_type) {
+      /* The component qualifier can't be used on non-numerical types so just
+       * treat all component slots as used. This will also make it so that
+       * any location aliasing attempt on non-numerical types is detected.
        */
       last_comp = 4;
    } else {
-      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
-      last_comp = component + type->without_array()->vector_elements * dmul;
+      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
+      last_comp = component + type_without_array->vector_elements * dmul;
    }
 
    while (location < location_limit) {
@@ -478,8 +496,18 @@  check_location_aliasing(struct explicit_location_info explicit_locations[][4],
             &explicit_locations[location][comp];
 
          if (info->var) {
-            /* Component aliasing is not alloed */
-            if (comp >= component && comp < last_comp) {
+            if (-1 == numerical_type || -1 == info->numerical_type) {
+               /* Location aliasing is only allowed for numerical variables */
+               linker_error(prog,
+                            "%s shader has location aliasing for "
+                            "non-numerical variable '%s'. Location %u "
+                            "component %u\n",
+                            _mesa_shader_stage_to_string(stage),
+                            -1 == numerical_type ? var->name : info->var->name,
+                            location, comp);
+               return false;
+            } else if (comp >= component && comp < last_comp) {
+               /* Component aliasing is not allowed */
                linker_error(prog,
                             "%s shader has multiple %sputs explicitly "
                             "assigned to location %d and component %d\n",
@@ -491,12 +519,12 @@  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->numerical_type !=
-                   get_numerical_type(type->without_array())) {
+               if (info->numerical_type != numerical_type) {
                   linker_error(prog,
-                               "Varyings sharing the same location must "
-                               "have the same underlying numerical type. "
-                               "Location %u component %u\n",
+                               "%s shader has varyings sharing the same "
+                               "location that don't have the same underlying "
+                               "numerical type. Location %u component %u\n",
+                               _mesa_shader_stage_to_string(stage),
                                location, comp);
                   return false;
                }
@@ -526,7 +554,7 @@  check_location_aliasing(struct explicit_location_info explicit_locations[][4],
             }
          } else if (comp >= component && comp < last_comp) {
             info->var = var;
-            info->numerical_type = get_numerical_type(type->without_array());
+            info->numerical_type = numerical_type;
             info->interpolation = interpolation;
             info->centroid = centroid;
             info->sample = sample;

Comments

On 2/2/19 5:05 am, Andres Gomez wrote:
> From: Iago Toral Quiroga <itoral@igalia.com>
> 
> Regarding location aliasing requirements, the OpenGL spec says:
> 
>    "Further, when location aliasing, the aliases sharing the location
>     must have the same underlying numerical type  (floating-point or
>     integer)."
> 
> Khronos has further clarified that this also requires the underlying
> types to have the same width, so we can't put a float and a double
> in the same location slot for example. Future versions of the spec will
> be corrected to make this clear.

The spec has always been very clear for example that it is ok to pack a 
float with a dvec3:

 From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:

"layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
                                 // and components 0 and 1 of location 9
  layout(location=9, component=2) in float i; // okay, compts 2 and 3"


Your going to need more information bug number etc to convince me this 
change is correct.

> 
> This patch amends our implementation to account for this restriction.
> 
> In the process of doing this, I also noticed that we would attempt
> to check aliasing requirements for record variables (including the test
> for the numerical type) which is not allowed, instead, we should be
> producing a linker error as soon as we see any attempt to do location
> aliasing on non-numerical variables. For the particular case of structs,
> we were producing a linker error in this case, but only because we
> assumed that struct fields use all components in each location, so
> any attempt to alias locations consumed by struct fields would produce
> a link error due to component aliasing, which is not accurate of the
> actual problem. This patch would make it produce an error for attempting
> to alias a non-numerical variable instead, which is always accurate.

None of this should be needed at all. It is an error to use 
location/component layouts on struct members.

"It is a compile-time error to use a location qualifier on a member of a 
structure."

"It is a compile-time error to use *component* without also specifying 
*location*"

So depending on the component aliasing check is sufficient. If you 
really want to add a better error message then see my comments below.


> 
> v2:
>    - Do not assert if we see invalid numerical types. These come
>      straight from shader code, so we should produce linker errors if
>      shaders attempt to do location aliasing on variables that are not
>      numerical such as records.
>    - While we are at it, improve error reporting for the case of
>      numerical type mismatch to include the shader stage.
> 
> v3:
>    - Allow location aliasing of images and samplers. If we get these
>      it means bindless support is active and they should be handled
>      as 64-bit integers (Ilia)
>    - Make sure we produce link errors for any non-numerical type
>      for which we attempt location aliasing, not just structs.
> 
> v4:
>    - Rebased with minor fixes (Andres).
>    - Added fixing tag to the commit log (Andres).
> 
> Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Signed-off-by: Andres Gomez <agomez@igalia.com>
> ---
>   src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
>   1 file changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index 3969c0120b3..3f41832ac93 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
>   
>   struct explicit_location_info {
>      ir_variable *var;
> -   unsigned numerical_type;
> +   int numerical_type;
>      unsigned interpolation;
>      bool centroid;
>      bool sample;
>      bool patch;
>   };
>   
> -static inline unsigned
> -get_numerical_type(const glsl_type *type)
> +static inline int
> +get_numerical_sized_type(const glsl_type *type)
>   {
>      /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
>       * (Location aliasing):
> @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
>       *    "Further, when location aliasing, the aliases sharing the location
>       *     must have the same underlying numerical type  (floating-point or
>       *     integer)
> +    *
> +    * Khronos has further clarified that this also requires the underlying
> +    * types to have the same width, so we can't put a float and a double
> +    * in the same location slot for example. Future versions of the spec will
> +    * be corrected to make this clear.
> +    *
> +    * Notice that we allow location aliasing for bindless image/samplers too
> +    * since these are defined as 64-bit integers.
>       */
> -   if (type->is_float() || type->is_double())
> +   if (type->is_float())
>         return GLSL_TYPE_FLOAT;
> -   return GLSL_TYPE_INT;
> +   else if (type->is_integer())
> +      return GLSL_TYPE_INT;
> +   else if (type->is_double())
> +      return GLSL_TYPE_DOUBLE;
> +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
> +      return GLSL_TYPE_INT64;
> +
> +   return -1; /* Not a numerical type */
>   }
>   
>   static bool
> @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>                           gl_shader_stage stage)
>   {
>      unsigned last_comp;
> -   if (type->without_array()->is_record()) {
> -      /* The component qualifier can't be used on structs so just treat
> -       * all component slots as used.
> +   const glsl_type *type_without_array = type->without_array();
> +   int numerical_type = get_numerical_sized_type(type_without_array);
> +   if (-1 == numerical_type) {
> +      /* The component qualifier can't be used on non-numerical types so just
> +       * treat all component slots as used. This will also make it so that
> +       * any location aliasing attempt on non-numerical types is detected.
>          */
>         last_comp = 4;
>      } else {
> -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> -      last_comp = component + type->without_array()->vector_elements * dmul;
> +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
> +      last_comp = component + type_without_array->vector_elements * dmul;
>      }
>   
>      while (location < location_limit) {
> @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>               &explicit_locations[location][comp];
>   
>            if (info->var) {
> -            /* Component aliasing is not alloed */
> -            if (comp >= component && comp < last_comp) {
> +            if (-1 == numerical_type || -1 == info->numerical_type) {

If you really want a new message for trying to pack with a struct then 
all you really need here is:

if (info->var->type->without_array()->is_record())

Then you can skip all this nasty -1 stuff.

> +               /* Location aliasing is only allowed for numerical variables */
> +               linker_error(prog,
> +                            "%s shader has location aliasing for "
> +                            "non-numerical variable '%s'. Location %u "
> +                            "component %u\n",
> +                            _mesa_shader_stage_to_string(stage),
> +                            -1 == numerical_type ? var->name : info->var->name,
> +                            location, comp);
> +               return false;
> +            } else if (comp >= component && comp < last_comp) {
> +               /* Component aliasing is not allowed */
>                  linker_error(prog,
>                               "%s shader has multiple %sputs explicitly "
>                               "assigned to location %d and component %d\n",
> @@ -491,12 +519,12 @@ 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->numerical_type !=
> -                   get_numerical_type(type->without_array())) {
> +               if (info->numerical_type != numerical_type) {
>                     linker_error(prog,
> -                               "Varyings sharing the same location must "
> -                               "have the same underlying numerical type. "
> -                               "Location %u component %u\n",
> +                               "%s shader has varyings sharing the same "
> +                               "location that don't have the same underlying "
> +                               "numerical type. Location %u component %u\n",
> +                               _mesa_shader_stage_to_string(stage),
>                                  location, comp);
>                     return false;
>                  }
> @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>               }
>            } else if (comp >= component && comp < last_comp) {
>               info->var = var;
> -            info->numerical_type = get_numerical_type(type->without_array());
> +            info->numerical_type = numerical_type;
>               info->interpolation = interpolation;
>               info->centroid = centroid;
>               info->sample = sample;
>
On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
> On 2/2/19 5:05 am, Andres Gomez wrote:
> > From: Iago Toral Quiroga <itoral@igalia.com>
> > 
> > Regarding location aliasing requirements, the OpenGL spec says:
> > 
> >    "Further, when location aliasing, the aliases sharing the location
> >     must have the same underlying numerical type  (floating-point or
> >     integer)."
> > 
> > Khronos has further clarified that this also requires the underlying
> > types to have the same width, so we can't put a float and a double
> > in the same location slot for example. Future versions of the spec will
> > be corrected to make this clear.
> 
> The spec has always been very clear for example that it is ok to pack a 
> float with a dvec3:
> 
>  From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
> 
> "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
>                                  // and components 0 and 1 of location 9
>   layout(location=9, component=2) in float i; // okay, compts 2 and 3"

Mmmm ... I didn't notice that example before. I think it is just
incorrect or, rather, a typo. The inline comment says that the float
takes components 2 and 3. A float will count only for *1* component.
Hence, the intended type there is a double, in which case the aliasing
is allowed.

The spec is actually very clear just some paragraphs below:

From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:

  " Further, when location aliasing, the aliases sharing the location
    must have the same underlying numerical type and bit
    width (floating-point or integer, 32-bit versus 64-bit, etc.)
    and the same auxiliary storage and interpolation
    qualification. The one exception where component aliasing is
    permitted is for two input variables (not block members) to a
    vertex shader, which are allowed to have component aliasing. This
    vertex-variable component aliasing is intended only to support
    vertex shaders where each execution path accesses at most one
    input per each aliased component. Implementations are permitted,
    but not required, to generate link-time errors if they detect
    that every path through the vertex shader executable accesses
    multiple inputs aliased to any single component."

> Your going to need more information bug number etc to convince me this 
> change is correct.

I'll file a bug against the specs.

In the meanwhile, this is the expected behavior also in the CTS tests,
which is also confirmed with the nVIDIA blob.

> 
> > This patch amends our implementation to account for this restriction.
> > 
> > In the process of doing this, I also noticed that we would attempt
> > to check aliasing requirements for record variables (including the test
> > for the numerical type) which is not allowed, instead, we should be
> > producing a linker error as soon as we see any attempt to do location
> > aliasing on non-numerical variables. For the particular case of structs,
> > we were producing a linker error in this case, but only because we
> > assumed that struct fields use all components in each location, so
> > any attempt to alias locations consumed by struct fields would produce
> > a link error due to component aliasing, which is not accurate of the
> > actual problem. This patch would make it produce an error for attempting
> > to alias a non-numerical variable instead, which is always accurate.
> 
> None of this should be needed at all. It is an error to use 
> location/component layouts on struct members.
> 
> "It is a compile-time error to use a location qualifier on a member of a 
> structure."
> 
> "It is a compile-time error to use *component* without also specifying 
> *location*"
> 
> So depending on the component aliasing check is sufficient. If you 
> really want to add a better error message then see my comments below.
> 
> 
> > v2:
> >    - Do not assert if we see invalid numerical types. These come
> >      straight from shader code, so we should produce linker errors if
> >      shaders attempt to do location aliasing on variables that are not
> >      numerical such as records.
> >    - While we are at it, improve error reporting for the case of
> >      numerical type mismatch to include the shader stage.
> > 
> > v3:
> >    - Allow location aliasing of images and samplers. If we get these
> >      it means bindless support is active and they should be handled
> >      as 64-bit integers (Ilia)
> >    - Make sure we produce link errors for any non-numerical type
> >      for which we attempt location aliasing, not just structs.
> > 
> > v4:
> >    - Rebased with minor fixes (Andres).
> >    - Added fixing tag to the commit log (Andres).
> > 
> > Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Signed-off-by: Andres Gomez <agomez@igalia.com>
> > ---
> >   src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
> >   1 file changed, 46 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> > index 3969c0120b3..3f41832ac93 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
> >   
> >   struct explicit_location_info {
> >      ir_variable *var;
> > -   unsigned numerical_type;
> > +   int numerical_type;
> >      unsigned interpolation;
> >      bool centroid;
> >      bool sample;
> >      bool patch;
> >   };
> >   
> > -static inline unsigned
> > -get_numerical_type(const glsl_type *type)
> > +static inline int
> > +get_numerical_sized_type(const glsl_type *type)
> >   {
> >      /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
> >       * (Location aliasing):
> > @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
> >       *    "Further, when location aliasing, the aliases sharing the location
> >       *     must have the same underlying numerical type  (floating-point or
> >       *     integer)
> > +    *
> > +    * Khronos has further clarified that this also requires the underlying
> > +    * types to have the same width, so we can't put a float and a double
> > +    * in the same location slot for example. Future versions of the spec will
> > +    * be corrected to make this clear.
> > +    *
> > +    * Notice that we allow location aliasing for bindless image/samplers too
> > +    * since these are defined as 64-bit integers.
> >       */
> > -   if (type->is_float() || type->is_double())
> > +   if (type->is_float())
> >         return GLSL_TYPE_FLOAT;
> > -   return GLSL_TYPE_INT;
> > +   else if (type->is_integer())
> > +      return GLSL_TYPE_INT;
> > +   else if (type->is_double())
> > +      return GLSL_TYPE_DOUBLE;
> > +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
> > +      return GLSL_TYPE_INT64;
> > +
> > +   return -1; /* Not a numerical type */
> >   }
> >   
> >   static bool
> > @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >                           gl_shader_stage stage)
> >   {
> >      unsigned last_comp;
> > -   if (type->without_array()->is_record()) {
> > -      /* The component qualifier can't be used on structs so just treat
> > -       * all component slots as used.
> > +   const glsl_type *type_without_array = type->without_array();
> > +   int numerical_type = get_numerical_sized_type(type_without_array);
> > +   if (-1 == numerical_type) {
> > +      /* The component qualifier can't be used on non-numerical types so just
> > +       * treat all component slots as used. This will also make it so that
> > +       * any location aliasing attempt on non-numerical types is detected.
> >          */
> >         last_comp = 4;
> >      } else {
> > -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> > -      last_comp = component + type->without_array()->vector_elements * dmul;
> > +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
> > +      last_comp = component + type_without_array->vector_elements * dmul;
> >      }
> >   
> >      while (location < location_limit) {
> > @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >               &explicit_locations[location][comp];
> >   
> >            if (info->var) {
> > -            /* Component aliasing is not alloed */
> > -            if (comp >= component && comp < last_comp) {
> > +            if (-1 == numerical_type || -1 == info->numerical_type) {
> 
> If you really want a new message for trying to pack with a struct then 
> all you really need here is:
> 
> if (info->var->type->without_array()->is_record())
> 
> Then you can skip all this nasty -1 stuff.
> 
> > +               /* Location aliasing is only allowed for numerical variables */
> > +               linker_error(prog,
> > +                            "%s shader has location aliasing for "
> > +                            "non-numerical variable '%s'. Location %u "
> > +                            "component %u\n",
> > +                            _mesa_shader_stage_to_string(stage),
> > +                            -1 == numerical_type ? var->name : info->var->name,
> > +                            location, comp);
> > +               return false;
> > +            } else if (comp >= component && comp < last_comp) {
> > +               /* Component aliasing is not allowed */
> >                  linker_error(prog,
> >                               "%s shader has multiple %sputs explicitly "
> >                               "assigned to location %d and component %d\n",
> > @@ -491,12 +519,12 @@ 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->numerical_type !=
> > -                   get_numerical_type(type->without_array())) {
> > +               if (info->numerical_type != numerical_type) {
> >                     linker_error(prog,
> > -                               "Varyings sharing the same location must "
> > -                               "have the same underlying numerical type. "
> > -                               "Location %u component %u\n",
> > +                               "%s shader has varyings sharing the same "
> > +                               "location that don't have the same underlying "
> > +                               "numerical type. Location %u component %u\n",
> > +                               _mesa_shader_stage_to_string(stage),
> >                                  location, comp);
> >                     return false;
> >                  }
> > @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >               }
> >            } else if (comp >= component && comp < last_comp) {
> >               info->var = var;
> > -            info->numerical_type = get_numerical_type(type->without_array());
> > +            info->numerical_type = numerical_type;
> >               info->interpolation = interpolation;
> >               info->centroid = centroid;
> >               info->sample = sample;
> >
On 20/3/19 9:31 pm, Andres Gomez wrote:
> On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
>> On 2/2/19 5:05 am, Andres Gomez wrote:
>>> From: Iago Toral Quiroga <itoral@igalia.com>
>>>
>>> Regarding location aliasing requirements, the OpenGL spec says:
>>>
>>>     "Further, when location aliasing, the aliases sharing the location
>>>      must have the same underlying numerical type  (floating-point or
>>>      integer)."
>>>
>>> Khronos has further clarified that this also requires the underlying
>>> types to have the same width, so we can't put a float and a double
>>> in the same location slot for example. Future versions of the spec will
>>> be corrected to make this clear.
>>
>> The spec has always been very clear for example that it is ok to pack a
>> float with a dvec3:
>>
>>   From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
>>
>> "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
>>                                   // and components 0 and 1 of location 9
>>    layout(location=9, component=2) in float i; // okay, compts 2 and 3"
> 
> Mmmm ... I didn't notice that example before. I think it is just
> incorrect or, rather, a typo. The inline comment says that the float
> takes components 2 and 3. A float will count only for *1* component.
> Hence, the intended type there is a double, in which case the aliasing
> is allowed.
> 
> The spec is actually very clear just some paragraphs below:
> 
>  From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:
> 
>    " Further, when location aliasing, the aliases sharing the location
>      must have the same underlying numerical type and bit
>      width (floating-point or integer, 32-bit versus 64-bit, etc.)
>      and the same auxiliary storage and interpolation
>      qualification. The one exception where component aliasing is
>      permitted is for two input variables (not block members) to a
>      vertex shader, which are allowed to have component aliasing. This
>      vertex-variable component aliasing is intended only to support
>      vertex shaders where each execution path accesses at most one
>      input per each aliased component. Implementations are permitted,
>      but not required, to generate link-time errors if they detect
>      that every path through the vertex shader executable accesses
>      multiple inputs aliased to any single component."

Yeah I noticed this when reviewing the piglit tests. It does seem we 
need to fix this. However rather than a custom 
get_numerical_sized_type() function we should be able to scrap it 
completely and possibly future proof the code by using:

glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead 
for the checks.

e.g.

info->is_integer = 
glsl_base_type_is_integer(type->without_array()->base_type);
info->bit_size = 
glsl_base_type_get_bit_size(type->without_array()->base_type);

And compare these instead.

> 
>> Your going to need more information bug number etc to convince me this
>> change is correct.
> 
> I'll file a bug against the specs.
> 
> In the meanwhile, this is the expected behavior also in the CTS tests,
> which is also confirmed with the nVIDIA blob.
> 
>>
>>> This patch amends our implementation to account for this restriction.
>>>
>>> In the process of doing this, I also noticed that we would attempt
>>> to check aliasing requirements for record variables (including the test
>>> for the numerical type) which is not allowed, instead, we should be
>>> producing a linker error as soon as we see any attempt to do location
>>> aliasing on non-numerical variables. For the particular case of structs,
>>> we were producing a linker error in this case, but only because we
>>> assumed that struct fields use all components in each location, so
>>> any attempt to alias locations consumed by struct fields would produce
>>> a link error due to component aliasing, which is not accurate of the
>>> actual problem. This patch would make it produce an error for attempting
>>> to alias a non-numerical variable instead, which is always accurate.
>>
>> None of this should be needed at all. It is an error to use
>> location/component layouts on struct members.
>>
>> "It is a compile-time error to use a location qualifier on a member of a
>> structure."
>>
>> "It is a compile-time error to use *component* without also specifying
>> *location*"
>>
>> So depending on the component aliasing check is sufficient. If you
>> really want to add a better error message then see my comments below.
>>
>>
>>> v2:
>>>     - Do not assert if we see invalid numerical types. These come
>>>       straight from shader code, so we should produce linker errors if
>>>       shaders attempt to do location aliasing on variables that are not
>>>       numerical such as records.
>>>     - While we are at it, improve error reporting for the case of
>>>       numerical type mismatch to include the shader stage.
>>>
>>> v3:
>>>     - Allow location aliasing of images and samplers. If we get these
>>>       it means bindless support is active and they should be handled
>>>       as 64-bit integers (Ilia)
>>>     - Make sure we produce link errors for any non-numerical type
>>>       for which we attempt location aliasing, not just structs.
>>>
>>> v4:
>>>     - Rebased with minor fixes (Andres).
>>>     - Added fixing tag to the commit log (Andres).
>>>
>>> Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
>>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>>> Signed-off-by: Andres Gomez <agomez@igalia.com>
>>> ---
>>>    src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
>>>    1 file changed, 46 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
>>> index 3969c0120b3..3f41832ac93 100644
>>> --- a/src/compiler/glsl/link_varyings.cpp
>>> +++ b/src/compiler/glsl/link_varyings.cpp
>>> @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
>>>    
>>>    struct explicit_location_info {
>>>       ir_variable *var;
>>> -   unsigned numerical_type;
>>> +   int numerical_type;
>>>       unsigned interpolation;
>>>       bool centroid;
>>>       bool sample;
>>>       bool patch;
>>>    };
>>>    
>>> -static inline unsigned
>>> -get_numerical_type(const glsl_type *type)
>>> +static inline int
>>> +get_numerical_sized_type(const glsl_type *type)
>>>    {
>>>       /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
>>>        * (Location aliasing):
>>> @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
>>>        *    "Further, when location aliasing, the aliases sharing the location
>>>        *     must have the same underlying numerical type  (floating-point or
>>>        *     integer)
>>> +    *
>>> +    * Khronos has further clarified that this also requires the underlying
>>> +    * types to have the same width, so we can't put a float and a double
>>> +    * in the same location slot for example. Future versions of the spec will
>>> +    * be corrected to make this clear.
>>> +    *
>>> +    * Notice that we allow location aliasing for bindless image/samplers too
>>> +    * since these are defined as 64-bit integers.
>>>        */
>>> -   if (type->is_float() || type->is_double())
>>> +   if (type->is_float())
>>>          return GLSL_TYPE_FLOAT;
>>> -   return GLSL_TYPE_INT;
>>> +   else if (type->is_integer())
>>> +      return GLSL_TYPE_INT;
>>> +   else if (type->is_double())
>>> +      return GLSL_TYPE_DOUBLE;
>>> +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
>>> +      return GLSL_TYPE_INT64;
>>> +
>>> +   return -1; /* Not a numerical type */
>>>    }
>>>    
>>>    static bool
>>> @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>                            gl_shader_stage stage)
>>>    {
>>>       unsigned last_comp;
>>> -   if (type->without_array()->is_record()) {
>>> -      /* The component qualifier can't be used on structs so just treat
>>> -       * all component slots as used.
>>> +   const glsl_type *type_without_array = type->without_array();
>>> +   int numerical_type = get_numerical_sized_type(type_without_array);
>>> +   if (-1 == numerical_type) {
>>> +      /* The component qualifier can't be used on non-numerical types so just
>>> +       * treat all component slots as used. This will also make it so that
>>> +       * any location aliasing attempt on non-numerical types is detected.
>>>           */
>>>          last_comp = 4;
>>>       } else {
>>> -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
>>> -      last_comp = component + type->without_array()->vector_elements * dmul;
>>> +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
>>> +      last_comp = component + type_without_array->vector_elements * dmul;
>>>       }
>>>    
>>>       while (location < location_limit) {
>>> @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>                &explicit_locations[location][comp];
>>>    
>>>             if (info->var) {
>>> -            /* Component aliasing is not alloed */
>>> -            if (comp >= component && comp < last_comp) {
>>> +            if (-1 == numerical_type || -1 == info->numerical_type) {
>>
>> If you really want a new message for trying to pack with a struct then
>> all you really need here is:
>>
>> if (info->var->type->without_array()->is_record())
>>
>> Then you can skip all this nasty -1 stuff.
>>
>>> +               /* Location aliasing is only allowed for numerical variables */
>>> +               linker_error(prog,
>>> +                            "%s shader has location aliasing for "
>>> +                            "non-numerical variable '%s'. Location %u "
>>> +                            "component %u\n",
>>> +                            _mesa_shader_stage_to_string(stage),
>>> +                            -1 == numerical_type ? var->name : info->var->name,
>>> +                            location, comp);
>>> +               return false;
>>> +            } else if (comp >= component && comp < last_comp) {
>>> +               /* Component aliasing is not allowed */
>>>                   linker_error(prog,
>>>                                "%s shader has multiple %sputs explicitly "
>>>                                "assigned to location %d and component %d\n",
>>> @@ -491,12 +519,12 @@ 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->numerical_type !=
>>> -                   get_numerical_type(type->without_array())) {
>>> +               if (info->numerical_type != numerical_type) {
>>>                      linker_error(prog,
>>> -                               "Varyings sharing the same location must "
>>> -                               "have the same underlying numerical type. "
>>> -                               "Location %u component %u\n",
>>> +                               "%s shader has varyings sharing the same "
>>> +                               "location that don't have the same underlying "
>>> +                               "numerical type. Location %u component %u\n",
>>> +                               _mesa_shader_stage_to_string(stage),
>>>                                   location, comp);
>>>                      return false;
>>>                   }
>>> @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>                }
>>>             } else if (comp >= component && comp < last_comp) {
>>>                info->var = var;
>>> -            info->numerical_type = get_numerical_type(type->without_array());
>>> +            info->numerical_type = numerical_type;
>>>                info->interpolation = interpolation;
>>>                info->centroid = centroid;
>>>                info->sample = sample;
>>>
On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
> On 2/2/19 5:05 am, Andres Gomez wrote:
> > From: Iago Toral Quiroga <itoral@igalia.com>
> > 
> > Regarding location aliasing requirements, the OpenGL spec says:
> > 
> >    "Further, when location aliasing, the aliases sharing the location
> >     must have the same underlying numerical type  (floating-point or
> >     integer)."
> > 
> > Khronos has further clarified that this also requires the underlying
> > types to have the same width, so we can't put a float and a double
> > in the same location slot for example. Future versions of the spec will
> > be corrected to make this clear.
> 
> The spec has always been very clear for example that it is ok to pack a 
> float with a dvec3:
> 
>  From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
> 
> "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
>                                  // and components 0 and 1 of location 9
>   layout(location=9, component=2) in float i; // okay, compts 2 and 3"
> 
> 
> Your going to need more information bug number etc to convince me this 
> change is correct.
> 
> > This patch amends our implementation to account for this restriction.
> > 
> > In the process of doing this, I also noticed that we would attempt
> > to check aliasing requirements for record variables (including the test
> > for the numerical type) which is not allowed, instead, we should be
> > producing a linker error as soon as we see any attempt to do location
> > aliasing on non-numerical variables. For the particular case of structs,
> > we were producing a linker error in this case, but only because we
> > assumed that struct fields use all components in each location, so
> > any attempt to alias locations consumed by struct fields would produce
> > a link error due to component aliasing, which is not accurate of the
> > actual problem. This patch would make it produce an error for attempting
> > to alias a non-numerical variable instead, which is always accurate.
> 
> None of this should be needed at all. It is an error to use 
> location/component layouts on struct members.
> 
> "It is a compile-time error to use a location qualifier on a member of a 
> structure."
> 
> "It is a compile-time error to use *component* without also specifying 
> *location*"
> 
> So depending on the component aliasing check is sufficient. If you 
> really want to add a better error message then see my comments below.

I believe this is already answered in the previous (unfinished) review
process through which this patch went through. Specifically, the 2nd
review that gave place to the v3 patch. You can check it here:

https://lists.freedesktop.org/archives/mesa-dev/2017-November/175366.html
https://lists.freedesktop.org/archives/mesa-dev/2017-November/175573.html
https://lists.freedesktop.org/archives/mesa-dev/2017-November/175705.html


> > v2:
> >    - Do not assert if we see invalid numerical types. These come
> >      straight from shader code, so we should produce linker errors if
> >      shaders attempt to do location aliasing on variables that are not
> >      numerical such as records.
> >    - While we are at it, improve error reporting for the case of
> >      numerical type mismatch to include the shader stage.
> > 
> > v3:
> >    - Allow location aliasing of images and samplers. If we get these
> >      it means bindless support is active and they should be handled
> >      as 64-bit integers (Ilia)
> >    - Make sure we produce link errors for any non-numerical type
> >      for which we attempt location aliasing, not just structs.
> > 
> > v4:
> >    - Rebased with minor fixes (Andres).
> >    - Added fixing tag to the commit log (Andres).
> > 
> > Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Signed-off-by: Andres Gomez <agomez@igalia.com>
> > ---
> >   src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
> >   1 file changed, 46 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> > index 3969c0120b3..3f41832ac93 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
> >   
> >   struct explicit_location_info {
> >      ir_variable *var;
> > -   unsigned numerical_type;
> > +   int numerical_type;
> >      unsigned interpolation;
> >      bool centroid;
> >      bool sample;
> >      bool patch;
> >   };
> >   
> > -static inline unsigned
> > -get_numerical_type(const glsl_type *type)
> > +static inline int
> > +get_numerical_sized_type(const glsl_type *type)
> >   {
> >      /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
> >       * (Location aliasing):
> > @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
> >       *    "Further, when location aliasing, the aliases sharing the location
> >       *     must have the same underlying numerical type  (floating-point or
> >       *     integer)
> > +    *
> > +    * Khronos has further clarified that this also requires the underlying
> > +    * types to have the same width, so we can't put a float and a double
> > +    * in the same location slot for example. Future versions of the spec will
> > +    * be corrected to make this clear.
> > +    *
> > +    * Notice that we allow location aliasing for bindless image/samplers too
> > +    * since these are defined as 64-bit integers.
> >       */
> > -   if (type->is_float() || type->is_double())
> > +   if (type->is_float())
> >         return GLSL_TYPE_FLOAT;
> > -   return GLSL_TYPE_INT;
> > +   else if (type->is_integer())
> > +      return GLSL_TYPE_INT;
> > +   else if (type->is_double())
> > +      return GLSL_TYPE_DOUBLE;
> > +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
> > +      return GLSL_TYPE_INT64;
> > +
> > +   return -1; /* Not a numerical type */
> >   }
> >   
> >   static bool
> > @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >                           gl_shader_stage stage)
> >   {
> >      unsigned last_comp;
> > -   if (type->without_array()->is_record()) {
> > -      /* The component qualifier can't be used on structs so just treat
> > -       * all component slots as used.
> > +   const glsl_type *type_without_array = type->without_array();
> > +   int numerical_type = get_numerical_sized_type(type_without_array);
> > +   if (-1 == numerical_type) {
> > +      /* The component qualifier can't be used on non-numerical types so just
> > +       * treat all component slots as used. This will also make it so that
> > +       * any location aliasing attempt on non-numerical types is detected.
> >          */
> >         last_comp = 4;
> >      } else {
> > -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> > -      last_comp = component + type->without_array()->vector_elements * dmul;
> > +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
> > +      last_comp = component + type_without_array->vector_elements * dmul;
> >      }
> >   
> >      while (location < location_limit) {
> > @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >               &explicit_locations[location][comp];
> >   
> >            if (info->var) {
> > -            /* Component aliasing is not alloed */
> > -            if (comp >= component && comp < last_comp) {
> > +            if (-1 == numerical_type || -1 == info->numerical_type) {
> 
> If you really want a new message for trying to pack with a struct then 
> all you really need here is:
> 
> if (info->var->type->without_array()->is_record())
> 
> Then you can skip all this nasty -1 stuff.
> 
> > +               /* Location aliasing is only allowed for numerical variables */
> > +               linker_error(prog,
> > +                            "%s shader has location aliasing for "
> > +                            "non-numerical variable '%s'. Location %u "
> > +                            "component %u\n",
> > +                            _mesa_shader_stage_to_string(stage),
> > +                            -1 == numerical_type ? var->name : info->var->name,
> > +                            location, comp);
> > +               return false;
> > +            } else if (comp >= component && comp < last_comp) {
> > +               /* Component aliasing is not allowed */
> >                  linker_error(prog,
> >                               "%s shader has multiple %sputs explicitly "
> >                               "assigned to location %d and component %d\n",
> > @@ -491,12 +519,12 @@ 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->numerical_type !=
> > -                   get_numerical_type(type->without_array())) {
> > +               if (info->numerical_type != numerical_type) {
> >                     linker_error(prog,
> > -                               "Varyings sharing the same location must "
> > -                               "have the same underlying numerical type. "
> > -                               "Location %u component %u\n",
> > +                               "%s shader has varyings sharing the same "
> > +                               "location that don't have the same underlying "
> > +                               "numerical type. Location %u component %u\n",
> > +                               _mesa_shader_stage_to_string(stage),
> >                                  location, comp);
> >                     return false;
> >                  }
> > @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >               }
> >            } else if (comp >= component && comp < last_comp) {
> >               info->var = var;
> > -            info->numerical_type = get_numerical_type(type->without_array());
> > +            info->numerical_type = numerical_type;
> >               info->interpolation = interpolation;
> >               info->centroid = centroid;
> >               info->sample = sample;
> >
On Thu, 2019-03-21 at 00:20 +1100, Timothy Arceri wrote:
> On 20/3/19 9:31 pm, Andres Gomez wrote:
> > On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
> > > On 2/2/19 5:05 am, Andres Gomez wrote:
> > > > From: Iago Toral Quiroga <itoral@igalia.com>
> > > > 
> > > > Regarding location aliasing requirements, the OpenGL spec says:
> > > > 
> > > >     "Further, when location aliasing, the aliases sharing the location
> > > >      must have the same underlying numerical type  (floating-point or
> > > >      integer)."
> > > > 
> > > > Khronos has further clarified that this also requires the underlying
> > > > types to have the same width, so we can't put a float and a double
> > > > in the same location slot for example. Future versions of the spec will
> > > > be corrected to make this clear.
> > > 
> > > The spec has always been very clear for example that it is ok to pack a
> > > float with a dvec3:
> > > 
> > >   From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
> > > 
> > > "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
> > >                                   // and components 0 and 1 of location 9
> > >    layout(location=9, component=2) in float i; // okay, compts 2 and 3"
> > 
> > Mmmm ... I didn't notice that example before. I think it is just
> > incorrect or, rather, a typo. The inline comment says that the float
> > takes components 2 and 3. A float will count only for *1* component.
> > Hence, the intended type there is a double, in which case the aliasing
> > is allowed.
> > 
> > The spec is actually very clear just some paragraphs below:
> > 
> >  From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:
> > 
> >    " Further, when location aliasing, the aliases sharing the location
> >      must have the same underlying numerical type and bit
> >      width (floating-point or integer, 32-bit versus 64-bit, etc.)
> >      and the same auxiliary storage and interpolation
> >      qualification. The one exception where component aliasing is
> >      permitted is for two input variables (not block members) to a
> >      vertex shader, which are allowed to have component aliasing. This
> >      vertex-variable component aliasing is intended only to support
> >      vertex shaders where each execution path accesses at most one
> >      input per each aliased component. Implementations are permitted,
> >      but not required, to generate link-time errors if they detect
> >      that every path through the vertex shader executable accesses
> >      multiple inputs aliased to any single component."
> 
> Yeah I noticed this when reviewing the piglit tests. It does seem we 
> need to fix this. However rather than a custom 
> get_numerical_sized_type() function we should be able to scrap it 
> completely and possibly future proof the code by using:
> 
> glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead 
> for the checks.
> 
> e.g.
> 
> info->is_integer = 
> glsl_base_type_is_integer(type->without_array()->base_type);
> info->bit_size = 
> glsl_base_type_get_bit_size(type->without_array()->base_type);
> 
> And compare these instead.

I don't think this is a better option. Some of the reasons, as
commented in my other mail in this thread, are explained in the
previous (unfinished) review process for this patch.

Additionally, glsl_base_type_is_integer is only true for 32bit
integers. Also, only with these 2 checks, we would be failing for
images and samplers. Finally, we would end with quite a big comparison
that will have to be scattered in several points of the
check_location_aliasing function, making more difficult to understand
the logic behind it. 

This is all addressed at single point, the (already pre-existent)
get_numerical_sized_type function, which makes this easier to
understand and, IMHO, more future proof.

> 
> > > Your going to need more information bug number etc to convince me this
> > > change is correct.
> > 
> > I'll file a bug against the specs.
> > 
> > In the meanwhile, this is the expected behavior also in the CTS tests,
> > which is also confirmed with the nVIDIA blob.
> > 
> > > > This patch amends our implementation to account for this restriction.
> > > > 
> > > > In the process of doing this, I also noticed that we would attempt
> > > > to check aliasing requirements for record variables (including the test
> > > > for the numerical type) which is not allowed, instead, we should be
> > > > producing a linker error as soon as we see any attempt to do location
> > > > aliasing on non-numerical variables. For the particular case of structs,
> > > > we were producing a linker error in this case, but only because we
> > > > assumed that struct fields use all components in each location, so
> > > > any attempt to alias locations consumed by struct fields would produce
> > > > a link error due to component aliasing, which is not accurate of the
> > > > actual problem. This patch would make it produce an error for attempting
> > > > to alias a non-numerical variable instead, which is always accurate.
> > > 
> > > None of this should be needed at all. It is an error to use
> > > location/component layouts on struct members.
> > > 
> > > "It is a compile-time error to use a location qualifier on a member of a
> > > structure."
> > > 
> > > "It is a compile-time error to use *component* without also specifying
> > > *location*"
> > > 
> > > So depending on the component aliasing check is sufficient. If you
> > > really want to add a better error message then see my comments below.
> > > 
> > > 
> > > > v2:
> > > >     - Do not assert if we see invalid numerical types. These come
> > > >       straight from shader code, so we should produce linker errors if
> > > >       shaders attempt to do location aliasing on variables that are not
> > > >       numerical such as records.
> > > >     - While we are at it, improve error reporting for the case of
> > > >       numerical type mismatch to include the shader stage.
> > > > 
> > > > v3:
> > > >     - Allow location aliasing of images and samplers. If we get these
> > > >       it means bindless support is active and they should be handled
> > > >       as 64-bit integers (Ilia)
> > > >     - Make sure we produce link errors for any non-numerical type
> > > >       for which we attempt location aliasing, not just structs.
> > > > 
> > > > v4:
> > > >     - Rebased with minor fixes (Andres).
> > > >     - Added fixing tag to the commit log (Andres).
> > > > 
> > > > Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
> > > > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > > > Signed-off-by: Andres Gomez <agomez@igalia.com>
> > > > ---
> > > >    src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
> > > >    1 file changed, 46 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> > > > index 3969c0120b3..3f41832ac93 100644
> > > > --- a/src/compiler/glsl/link_varyings.cpp
> > > > +++ b/src/compiler/glsl/link_varyings.cpp
> > > > @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
> > > >    
> > > >    struct explicit_location_info {
> > > >       ir_variable *var;
> > > > -   unsigned numerical_type;
> > > > +   int numerical_type;
> > > >       unsigned interpolation;
> > > >       bool centroid;
> > > >       bool sample;
> > > >       bool patch;
> > > >    };
> > > >    
> > > > -static inline unsigned
> > > > -get_numerical_type(const glsl_type *type)
> > > > +static inline int
> > > > +get_numerical_sized_type(const glsl_type *type)
> > > >    {
> > > >       /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
> > > >        * (Location aliasing):
> > > > @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
> > > >        *    "Further, when location aliasing, the aliases sharing the location
> > > >        *     must have the same underlying numerical type  (floating-point or
> > > >        *     integer)
> > > > +    *
> > > > +    * Khronos has further clarified that this also requires the underlying
> > > > +    * types to have the same width, so we can't put a float and a double
> > > > +    * in the same location slot for example. Future versions of the spec will
> > > > +    * be corrected to make this clear.
> > > > +    *
> > > > +    * Notice that we allow location aliasing for bindless image/samplers too
> > > > +    * since these are defined as 64-bit integers.
> > > >        */
> > > > -   if (type->is_float() || type->is_double())
> > > > +   if (type->is_float())
> > > >          return GLSL_TYPE_FLOAT;
> > > > -   return GLSL_TYPE_INT;
> > > > +   else if (type->is_integer())
> > > > +      return GLSL_TYPE_INT;
> > > > +   else if (type->is_double())
> > > > +      return GLSL_TYPE_DOUBLE;
> > > > +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
> > > > +      return GLSL_TYPE_INT64;
> > > > +
> > > > +   return -1; /* Not a numerical type */
> > > >    }
> > > >    
> > > >    static bool
> > > > @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > >                            gl_shader_stage stage)
> > > >    {
> > > >       unsigned last_comp;
> > > > -   if (type->without_array()->is_record()) {
> > > > -      /* The component qualifier can't be used on structs so just treat
> > > > -       * all component slots as used.
> > > > +   const glsl_type *type_without_array = type->without_array();
> > > > +   int numerical_type = get_numerical_sized_type(type_without_array);
> > > > +   if (-1 == numerical_type) {
> > > > +      /* The component qualifier can't be used on non-numerical types so just
> > > > +       * treat all component slots as used. This will also make it so that
> > > > +       * any location aliasing attempt on non-numerical types is detected.
> > > >           */
> > > >          last_comp = 4;
> > > >       } else {
> > > > -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> > > > -      last_comp = component + type->without_array()->vector_elements * dmul;
> > > > +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
> > > > +      last_comp = component + type_without_array->vector_elements * dmul;
> > > >       }
> > > >    
> > > >       while (location < location_limit) {
> > > > @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > >                &explicit_locations[location][comp];
> > > >    
> > > >             if (info->var) {
> > > > -            /* Component aliasing is not alloed */
> > > > -            if (comp >= component && comp < last_comp) {
> > > > +            if (-1 == numerical_type || -1 == info->numerical_type) {
> > > 
> > > If you really want a new message for trying to pack with a struct then
> > > all you really need here is:
> > > 
> > > if (info->var->type->without_array()->is_record())
> > > 
> > > Then you can skip all this nasty -1 stuff.
> > > 
> > > > +               /* Location aliasing is only allowed for numerical variables */
> > > > +               linker_error(prog,
> > > > +                            "%s shader has location aliasing for "
> > > > +                            "non-numerical variable '%s'. Location %u "
> > > > +                            "component %u\n",
> > > > +                            _mesa_shader_stage_to_string(stage),
> > > > +                            -1 == numerical_type ? var->name : info->var->name,
> > > > +                            location, comp);
> > > > +               return false;
> > > > +            } else if (comp >= component && comp < last_comp) {
> > > > +               /* Component aliasing is not allowed */
> > > >                   linker_error(prog,
> > > >                                "%s shader has multiple %sputs explicitly "
> > > >                                "assigned to location %d and component %d\n",
> > > > @@ -491,12 +519,12 @@ 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->numerical_type !=
> > > > -                   get_numerical_type(type->without_array())) {
> > > > +               if (info->numerical_type != numerical_type) {
> > > >                      linker_error(prog,
> > > > -                               "Varyings sharing the same location must "
> > > > -                               "have the same underlying numerical type. "
> > > > -                               "Location %u component %u\n",
> > > > +                               "%s shader has varyings sharing the same "
> > > > +                               "location that don't have the same underlying "
> > > > +                               "numerical type. Location %u component %u\n",
> > > > +                               _mesa_shader_stage_to_string(stage),
> > > >                                   location, comp);
> > > >                      return false;
> > > >                   }
> > > > @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > >                }
> > > >             } else if (comp >= component && comp < last_comp) {
> > > >                info->var = var;
> > > > -            info->numerical_type = get_numerical_type(type->without_array());
> > > > +            info->numerical_type = numerical_type;
> > > >                info->interpolation = interpolation;
> > > >                info->centroid = centroid;
> > > >                info->sample = sample;
> > > >
On Wed, 2019-03-20 at 16:46 +0200, Andres Gomez wrote:
> On Thu, 2019-03-21 at 00:20 +1100, Timothy Arceri wrote:
> > On 20/3/19 9:31 pm, Andres Gomez wrote:
> > > On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
> > > > On 2/2/19 5:05 am, Andres Gomez wrote:
> > > > > From: Iago Toral Quiroga <itoral@igalia.com>
> > > > > 
> > > > > Regarding location aliasing requirements, the OpenGL spec says:
> > > > > 
> > > > >     "Further, when location aliasing, the aliases sharing the location
> > > > >      must have the same underlying numerical type  (floating-point or
> > > > >      integer)."
> > > > > 
> > > > > Khronos has further clarified that this also requires the underlying
> > > > > types to have the same width, so we can't put a float and a double
> > > > > in the same location slot for example. Future versions of the spec will
> > > > > be corrected to make this clear.
> > > > 
> > > > The spec has always been very clear for example that it is ok to pack a
> > > > float with a dvec3:
> > > > 
> > > >   From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
> > > > 
> > > > "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
> > > >                                   // and components 0 and 1 of location 9
> > > >    layout(location=9, component=2) in float i; // okay, compts 2 and 3"
> > > 
> > > Mmmm ... I didn't notice that example before. I think it is just
> > > incorrect or, rather, a typo. The inline comment says that the float
> > > takes components 2 and 3. A float will count only for *1* component.
> > > Hence, the intended type there is a double, in which case the aliasing
> > > is allowed.
> > > 
> > > The spec is actually very clear just some paragraphs below:
> > > 
> > >  From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:
> > > 
> > >    " Further, when location aliasing, the aliases sharing the location
> > >      must have the same underlying numerical type and bit
> > >      width (floating-point or integer, 32-bit versus 64-bit, etc.)
> > >      and the same auxiliary storage and interpolation
> > >      qualification. The one exception where component aliasing is
> > >      permitted is for two input variables (not block members) to a
> > >      vertex shader, which are allowed to have component aliasing. This
> > >      vertex-variable component aliasing is intended only to support
> > >      vertex shaders where each execution path accesses at most one
> > >      input per each aliased component. Implementations are permitted,
> > >      but not required, to generate link-time errors if they detect
> > >      that every path through the vertex shader executable accesses
> > >      multiple inputs aliased to any single component."
> > 
> > Yeah I noticed this when reviewing the piglit tests. It does seem we 
> > need to fix this. However rather than a custom 
> > get_numerical_sized_type() function we should be able to scrap it 
> > completely and possibly future proof the code by using:
> > 
> > glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead 
> > for the checks.
> > 
> > e.g.
> > 
> > info->is_integer = 
> > glsl_base_type_is_integer(type->without_array()->base_type);
> > info->bit_size = 
> > glsl_base_type_get_bit_size(type->without_array()->base_type);
> > 
> > And compare these instead.
> 
> I don't think this is a better option. Some of the reasons, as
> commented in my other mail in this thread, are explained in the
> previous (unfinished) review process for this patch.
> 
> Additionally, glsl_base_type_is_integer is only true for 32bit
> integers. Also, only with these 2 checks, we would be failing for
> images and samplers. Finally, we would end with quite a big comparison
> that will have to be scattered in several points of the
> check_location_aliasing function, making more difficult to understand
> the logic behind it. 

Scratch the last paragraph. I committed the mistake of checking
"is_integer" instead of "glsl_base_type_is_integer"

> This is all addressed at single point, the (already pre-existent)
> get_numerical_sized_type function, which makes this easier to
> understand and, IMHO, more future proof.

Even with the possibility of using those 2, I would rather keep the
get_numerical_sized_type to address the cases of "non-numerical"
variables beyond structs.

> 
> > > > Your going to need more information bug number etc to convince me this
> > > > change is correct.
> > > 
> > > I'll file a bug against the specs.
> > > 
> > > In the meanwhile, this is the expected behavior also in the CTS tests,
> > > which is also confirmed with the nVIDIA blob.
> > > 
> > > > > This patch amends our implementation to account for this restriction.
> > > > > 
> > > > > In the process of doing this, I also noticed that we would attempt
> > > > > to check aliasing requirements for record variables (including the test
> > > > > for the numerical type) which is not allowed, instead, we should be
> > > > > producing a linker error as soon as we see any attempt to do location
> > > > > aliasing on non-numerical variables. For the particular case of structs,
> > > > > we were producing a linker error in this case, but only because we
> > > > > assumed that struct fields use all components in each location, so
> > > > > any attempt to alias locations consumed by struct fields would produce
> > > > > a link error due to component aliasing, which is not accurate of the
> > > > > actual problem. This patch would make it produce an error for attempting
> > > > > to alias a non-numerical variable instead, which is always accurate.
> > > > 
> > > > None of this should be needed at all. It is an error to use
> > > > location/component layouts on struct members.
> > > > 
> > > > "It is a compile-time error to use a location qualifier on a member of a
> > > > structure."
> > > > 
> > > > "It is a compile-time error to use *component* without also specifying
> > > > *location*"
> > > > 
> > > > So depending on the component aliasing check is sufficient. If you
> > > > really want to add a better error message then see my comments below.
> > > > 
> > > > 
> > > > > v2:
> > > > >     - Do not assert if we see invalid numerical types. These come
> > > > >       straight from shader code, so we should produce linker errors if
> > > > >       shaders attempt to do location aliasing on variables that are not
> > > > >       numerical such as records.
> > > > >     - While we are at it, improve error reporting for the case of
> > > > >       numerical type mismatch to include the shader stage.
> > > > > 
> > > > > v3:
> > > > >     - Allow location aliasing of images and samplers. If we get these
> > > > >       it means bindless support is active and they should be handled
> > > > >       as 64-bit integers (Ilia)
> > > > >     - Make sure we produce link errors for any non-numerical type
> > > > >       for which we attempt location aliasing, not just structs.
> > > > > 
> > > > > v4:
> > > > >     - Rebased with minor fixes (Andres).
> > > > >     - Added fixing tag to the commit log (Andres).
> > > > > 
> > > > > Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
> > > > > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > > > > Signed-off-by: Andres Gomez <agomez@igalia.com>
> > > > > ---
> > > > >    src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
> > > > >    1 file changed, 46 insertions(+), 18 deletions(-)
> > > > > 
> > > > > diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> > > > > index 3969c0120b3..3f41832ac93 100644
> > > > > --- a/src/compiler/glsl/link_varyings.cpp
> > > > > +++ b/src/compiler/glsl/link_varyings.cpp
> > > > > @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
> > > > >    
> > > > >    struct explicit_location_info {
> > > > >       ir_variable *var;
> > > > > -   unsigned numerical_type;
> > > > > +   int numerical_type;
> > > > >       unsigned interpolation;
> > > > >       bool centroid;
> > > > >       bool sample;
> > > > >       bool patch;
> > > > >    };
> > > > >    
> > > > > -static inline unsigned
> > > > > -get_numerical_type(const glsl_type *type)
> > > > > +static inline int
> > > > > +get_numerical_sized_type(const glsl_type *type)
> > > > >    {
> > > > >       /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
> > > > >        * (Location aliasing):
> > > > > @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
> > > > >        *    "Further, when location aliasing, the aliases sharing the location
> > > > >        *     must have the same underlying numerical type  (floating-point or
> > > > >        *     integer)
> > > > > +    *
> > > > > +    * Khronos has further clarified that this also requires the underlying
> > > > > +    * types to have the same width, so we can't put a float and a double
> > > > > +    * in the same location slot for example. Future versions of the spec will
> > > > > +    * be corrected to make this clear.
> > > > > +    *
> > > > > +    * Notice that we allow location aliasing for bindless image/samplers too
> > > > > +    * since these are defined as 64-bit integers.
> > > > >        */
> > > > > -   if (type->is_float() || type->is_double())
> > > > > +   if (type->is_float())
> > > > >          return GLSL_TYPE_FLOAT;
> > > > > -   return GLSL_TYPE_INT;
> > > > > +   else if (type->is_integer())
> > > > > +      return GLSL_TYPE_INT;
> > > > > +   else if (type->is_double())
> > > > > +      return GLSL_TYPE_DOUBLE;
> > > > > +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
> > > > > +      return GLSL_TYPE_INT64;
> > > > > +
> > > > > +   return -1; /* Not a numerical type */
> > > > >    }
> > > > >    
> > > > >    static bool
> > > > > @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > > >                            gl_shader_stage stage)
> > > > >    {
> > > > >       unsigned last_comp;
> > > > > -   if (type->without_array()->is_record()) {
> > > > > -      /* The component qualifier can't be used on structs so just treat
> > > > > -       * all component slots as used.
> > > > > +   const glsl_type *type_without_array = type->without_array();
> > > > > +   int numerical_type = get_numerical_sized_type(type_without_array);
> > > > > +   if (-1 == numerical_type) {
> > > > > +      /* The component qualifier can't be used on non-numerical types so just
> > > > > +       * treat all component slots as used. This will also make it so that
> > > > > +       * any location aliasing attempt on non-numerical types is detected.
> > > > >           */
> > > > >          last_comp = 4;
> > > > >       } else {
> > > > > -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> > > > > -      last_comp = component + type->without_array()->vector_elements * dmul;
> > > > > +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
> > > > > +      last_comp = component + type_without_array->vector_elements * dmul;
> > > > >       }
> > > > >    
> > > > >       while (location < location_limit) {
> > > > > @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > > >                &explicit_locations[location][comp];
> > > > >    
> > > > >             if (info->var) {
> > > > > -            /* Component aliasing is not alloed */
> > > > > -            if (comp >= component && comp < last_comp) {
> > > > > +            if (-1 == numerical_type || -1 == info->numerical_type) {
> > > > 
> > > > If you really want a new message for trying to pack with a struct then
> > > > all you really need here is:
> > > > 
> > > > if (info->var->type->without_array()->is_record())
> > > > 
> > > > Then you can skip all this nasty -1 stuff.
> > > > 
> > > > > +               /* Location aliasing is only allowed for numerical variables */
> > > > > +               linker_error(prog,
> > > > > +                            "%s shader has location aliasing for "
> > > > > +                            "non-numerical variable '%s'. Location %u "
> > > > > +                            "component %u\n",
> > > > > +                            _mesa_shader_stage_to_string(stage),
> > > > > +                            -1 == numerical_type ? var->name : info->var->name,
> > > > > +                            location, comp);
> > > > > +               return false;
> > > > > +            } else if (comp >= component && comp < last_comp) {
> > > > > +               /* Component aliasing is not allowed */
> > > > >                   linker_error(prog,
> > > > >                                "%s shader has multiple %sputs explicitly "
> > > > >                                "assigned to location %d and component %d\n",
> > > > > @@ -491,12 +519,12 @@ 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->numerical_type !=
> > > > > -                   get_numerical_type(type->without_array())) {
> > > > > +               if (info->numerical_type != numerical_type) {
> > > > >                      linker_error(prog,
> > > > > -                               "Varyings sharing the same location must "
> > > > > -                               "have the same underlying numerical type. "
> > > > > -                               "Location %u component %u\n",
> > > > > +                               "%s shader has varyings sharing the same "
> > > > > +                               "location that don't have the same underlying "
> > > > > +                               "numerical type. Location %u component %u\n",
> > > > > +                               _mesa_shader_stage_to_string(stage),
> > > > >                                   location, comp);
> > > > >                      return false;
> > > > >                   }
> > > > > @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> > > > >                }
> > > > >             } else if (comp >= component && comp < last_comp) {
> > > > >                info->var = var;
> > > > > -            info->numerical_type = get_numerical_type(type->without_array());
> > > > > +            info->numerical_type = numerical_type;
> > > > >                info->interpolation = interpolation;
> > > > >                info->centroid = centroid;
> > > > >                info->sample = sample;
> > > > >
On 21/3/19 1:38 am, Andres Gomez wrote:
> On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
>> On 2/2/19 5:05 am, Andres Gomez wrote:
>>> From: Iago Toral Quiroga <itoral@igalia.com>
>>>
>>> Regarding location aliasing requirements, the OpenGL spec says:
>>>
>>>     "Further, when location aliasing, the aliases sharing the location
>>>      must have the same underlying numerical type  (floating-point or
>>>      integer)."
>>>
>>> Khronos has further clarified that this also requires the underlying
>>> types to have the same width, so we can't put a float and a double
>>> in the same location slot for example. Future versions of the spec will
>>> be corrected to make this clear.
>>
>> The spec has always been very clear for example that it is ok to pack a
>> float with a dvec3:
>>
>>   From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
>>
>> "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
>>                                   // and components 0 and 1 of location 9
>>    layout(location=9, component=2) in float i; // okay, compts 2 and 3"
>>
>>
>> Your going to need more information bug number etc to convince me this
>> change is correct.
>>
>>> This patch amends our implementation to account for this restriction.
>>>
>>> In the process of doing this, I also noticed that we would attempt
>>> to check aliasing requirements for record variables (including the test
>>> for the numerical type) which is not allowed, instead, we should be
>>> producing a linker error as soon as we see any attempt to do location
>>> aliasing on non-numerical variables. For the particular case of structs,
>>> we were producing a linker error in this case, but only because we
>>> assumed that struct fields use all components in each location, so
>>> any attempt to alias locations consumed by struct fields would produce
>>> a link error due to component aliasing, which is not accurate of the
>>> actual problem. This patch would make it produce an error for attempting
>>> to alias a non-numerical variable instead, which is always accurate.
>>
>> None of this should be needed at all. It is an error to use
>> location/component layouts on struct members.
>>
>> "It is a compile-time error to use a location qualifier on a member of a
>> structure."
>>
>> "It is a compile-time error to use *component* without also specifying
>> *location*"
>>
>> So depending on the component aliasing check is sufficient. If you
>> really want to add a better error message then see my comments below.
> 
> I believe this is already answered in the previous (unfinished) review
> process through which this patch went through. Specifically, the 2nd
> review that gave place to the v3 patch.

No that doesn't address my comment. It shows why you did what you did, 
but what I'm saying is you don't need to check for this type of thing. 
It should all already be handled by compile-time error checking. You are 
making things more complicated than they need to be.

If I'm wrong please point me to a piglit test that can't be resolved by 
simply checking for a struct as I suggest below.

> You can check it here:
> 
> https://lists.freedesktop.org/archives/mesa-dev/2017-November/175366.html
> https://lists.freedesktop.org/archives/mesa-dev/2017-November/175573.html
> https://lists.freedesktop.org/archives/mesa-dev/2017-November/175705.html
> 
> 
>>> v2:
>>>     - Do not assert if we see invalid numerical types. These come
>>>       straight from shader code, so we should produce linker errors if
>>>       shaders attempt to do location aliasing on variables that are not
>>>       numerical such as records.
>>>     - While we are at it, improve error reporting for the case of
>>>       numerical type mismatch to include the shader stage.
>>>
>>> v3:
>>>     - Allow location aliasing of images and samplers. If we get these
>>>       it means bindless support is active and they should be handled
>>>       as 64-bit integers (Ilia)
>>>     - Make sure we produce link errors for any non-numerical type
>>>       for which we attempt location aliasing, not just structs.
>>>
>>> v4:
>>>     - Rebased with minor fixes (Andres).
>>>     - Added fixing tag to the commit log (Andres).
>>>
>>> Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
>>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>>> Signed-off-by: Andres Gomez <agomez@igalia.com>
>>> ---
>>>    src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
>>>    1 file changed, 46 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
>>> index 3969c0120b3..3f41832ac93 100644
>>> --- a/src/compiler/glsl/link_varyings.cpp
>>> +++ b/src/compiler/glsl/link_varyings.cpp
>>> @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
>>>    
>>>    struct explicit_location_info {
>>>       ir_variable *var;
>>> -   unsigned numerical_type;
>>> +   int numerical_type;
>>>       unsigned interpolation;
>>>       bool centroid;
>>>       bool sample;
>>>       bool patch;
>>>    };
>>>    
>>> -static inline unsigned
>>> -get_numerical_type(const glsl_type *type)
>>> +static inline int
>>> +get_numerical_sized_type(const glsl_type *type)
>>>    {
>>>       /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
>>>        * (Location aliasing):
>>> @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
>>>        *    "Further, when location aliasing, the aliases sharing the location
>>>        *     must have the same underlying numerical type  (floating-point or
>>>        *     integer)
>>> +    *
>>> +    * Khronos has further clarified that this also requires the underlying
>>> +    * types to have the same width, so we can't put a float and a double
>>> +    * in the same location slot for example. Future versions of the spec will
>>> +    * be corrected to make this clear.
>>> +    *
>>> +    * Notice that we allow location aliasing for bindless image/samplers too
>>> +    * since these are defined as 64-bit integers.
>>>        */
>>> -   if (type->is_float() || type->is_double())
>>> +   if (type->is_float())
>>>          return GLSL_TYPE_FLOAT;
>>> -   return GLSL_TYPE_INT;
>>> +   else if (type->is_integer())
>>> +      return GLSL_TYPE_INT;
>>> +   else if (type->is_double())
>>> +      return GLSL_TYPE_DOUBLE;
>>> +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
>>> +      return GLSL_TYPE_INT64;
>>> +
>>> +   return -1; /* Not a numerical type */
>>>    }
>>>    
>>>    static bool
>>> @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>                            gl_shader_stage stage)
>>>    {
>>>       unsigned last_comp;
>>> -   if (type->without_array()->is_record()) {
>>> -      /* The component qualifier can't be used on structs so just treat
>>> -       * all component slots as used.
>>> +   const glsl_type *type_without_array = type->without_array();
>>> +   int numerical_type = get_numerical_sized_type(type_without_array);
>>> +   if (-1 == numerical_type) {
>>> +      /* The component qualifier can't be used on non-numerical types so just
>>> +       * treat all component slots as used. This will also make it so that
>>> +       * any location aliasing attempt on non-numerical types is detected.
>>>           */
>>>          last_comp = 4;
>>>       } else {
>>> -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
>>> -      last_comp = component + type->without_array()->vector_elements * dmul;
>>> +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
>>> +      last_comp = component + type_without_array->vector_elements * dmul;
>>>       }
>>>    
>>>       while (location < location_limit) {
>>> @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>                &explicit_locations[location][comp];
>>>    
>>>             if (info->var) {
>>> -            /* Component aliasing is not alloed */
>>> -            if (comp >= component && comp < last_comp) {
>>> +            if (-1 == numerical_type || -1 == info->numerical_type) {
>>
>> If you really want a new message for trying to pack with a struct then
>> all you really need here is:
>>
>> if (info->var->type->without_array()->is_record())
>>
>> Then you can skip all this nasty -1 stuff.
>>
>>> +               /* Location aliasing is only allowed for numerical variables */
>>> +               linker_error(prog,
>>> +                            "%s shader has location aliasing for "
>>> +                            "non-numerical variable '%s'. Location %u "
>>> +                            "component %u\n",
>>> +                            _mesa_shader_stage_to_string(stage),
>>> +                            -1 == numerical_type ? var->name : info->var->name,
>>> +                            location, comp);
>>> +               return false;
>>> +            } else if (comp >= component && comp < last_comp) {
>>> +               /* Component aliasing is not allowed */
>>>                   linker_error(prog,
>>>                                "%s shader has multiple %sputs explicitly "
>>>                                "assigned to location %d and component %d\n",
>>> @@ -491,12 +519,12 @@ 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->numerical_type !=
>>> -                   get_numerical_type(type->without_array())) {
>>> +               if (info->numerical_type != numerical_type) {
>>>                      linker_error(prog,
>>> -                               "Varyings sharing the same location must "
>>> -                               "have the same underlying numerical type. "
>>> -                               "Location %u component %u\n",
>>> +                               "%s shader has varyings sharing the same "
>>> +                               "location that don't have the same underlying "
>>> +                               "numerical type. Location %u component %u\n",
>>> +                               _mesa_shader_stage_to_string(stage),
>>>                                   location, comp);
>>>                      return false;
>>>                   }
>>> @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>                }
>>>             } else if (comp >= component && comp < last_comp) {
>>>                info->var = var;
>>> -            info->numerical_type = get_numerical_type(type->without_array());
>>> +            info->numerical_type = numerical_type;
>>>                info->interpolation = interpolation;
>>>                info->centroid = centroid;
>>>                info->sample = sample;
>>>
On 21/3/19 2:07 am, Andres Gomez wrote:
> On Wed, 2019-03-20 at 16:46 +0200, Andres Gomez wrote:
>> On Thu, 2019-03-21 at 00:20 +1100, Timothy Arceri wrote:
>>> On 20/3/19 9:31 pm, Andres Gomez wrote:
>>>> On Wed, 2019-03-20 at 20:46 +1100, Timothy Arceri wrote:
>>>>> On 2/2/19 5:05 am, Andres Gomez wrote:
>>>>>> From: Iago Toral Quiroga <itoral@igalia.com>
>>>>>>
>>>>>> Regarding location aliasing requirements, the OpenGL spec says:
>>>>>>
>>>>>>      "Further, when location aliasing, the aliases sharing the location
>>>>>>       must have the same underlying numerical type  (floating-point or
>>>>>>       integer)."
>>>>>>
>>>>>> Khronos has further clarified that this also requires the underlying
>>>>>> types to have the same width, so we can't put a float and a double
>>>>>> in the same location slot for example. Future versions of the spec will
>>>>>> be corrected to make this clear.
>>>>>
>>>>> The spec has always been very clear for example that it is ok to pack a
>>>>> float with a dvec3:
>>>>>
>>>>>    From section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.6 spec:
>>>>>
>>>>> "layout(location=8) in dvec3 h; // components 0,1,2 and 3 of location 8
>>>>>                                    // and components 0 and 1 of location 9
>>>>>     layout(location=9, component=2) in float i; // okay, compts 2 and 3"
>>>>
>>>> Mmmm ... I didn't notice that example before. I think it is just
>>>> incorrect or, rather, a typo. The inline comment says that the float
>>>> takes components 2 and 3. A float will count only for *1* component.
>>>> Hence, the intended type there is a double, in which case the aliasing
>>>> is allowed.
>>>>
>>>> The spec is actually very clear just some paragraphs below:
>>>>
>>>>   From Section 4.4.1 (Input Layout Qualifiers) of the GLSL 4.60 spec:
>>>>
>>>>     " Further, when location aliasing, the aliases sharing the location
>>>>       must have the same underlying numerical type and bit
>>>>       width (floating-point or integer, 32-bit versus 64-bit, etc.)
>>>>       and the same auxiliary storage and interpolation
>>>>       qualification. The one exception where component aliasing is
>>>>       permitted is for two input variables (not block members) to a
>>>>       vertex shader, which are allowed to have component aliasing. This
>>>>       vertex-variable component aliasing is intended only to support
>>>>       vertex shaders where each execution path accesses at most one
>>>>       input per each aliased component. Implementations are permitted,
>>>>       but not required, to generate link-time errors if they detect
>>>>       that every path through the vertex shader executable accesses
>>>>       multiple inputs aliased to any single component."
>>>
>>> Yeah I noticed this when reviewing the piglit tests. It does seem we
>>> need to fix this. However rather than a custom
>>> get_numerical_sized_type() function we should be able to scrap it
>>> completely and possibly future proof the code by using:
>>>
>>> glsl_base_type_get_bit_size() and glsl_base_type_is_integer() instead
>>> for the checks.
>>>
>>> e.g.
>>>
>>> info->is_integer =
>>> glsl_base_type_is_integer(type->without_array()->base_type);
>>> info->bit_size =
>>> glsl_base_type_get_bit_size(type->without_array()->base_type);
>>>
>>> And compare these instead.
>>
>> I don't think this is a better option. Some of the reasons, as
>> commented in my other mail in this thread, are explained in the
>> previous (unfinished) review process for this patch.
>>
>> Additionally, glsl_base_type_is_integer is only true for 32bit
>> integers. Also, only with these 2 checks, we would be failing for
>> images and samplers. Finally, we would end with quite a big comparison
>> that will have to be scattered in several points of the
>> check_location_aliasing function, making more difficult to understand
>> the logic behind it.

I disagree. IMO it would actually be much easier to follow and you can 
make the error message much more precise by including the bit-size and 
integer vs float difference.

> 
> Scratch the last paragraph. I committed the mistake of checking
> "is_integer" instead of "glsl_base_type_is_integer"
> 
>> This is all addressed at single point, the (already pre-existent)
>> get_numerical_sized_type function, which makes this easier to
>> understand and, IMHO, more future proof.
> 
> Even with the possibility of using those 2, I would rather keep the
> get_numerical_sized_type to address the cases of "non-numerical"
> variables beyond structs.

As per my reply to the other thread you shouldn't need to be testing for 
anything other than structs, everything else should already have been 
disallowed by compile-time errors. If I'm wrong can you please give me 
an example of what you are trying to fix.

> 
>>
>>>>> Your going to need more information bug number etc to convince me this
>>>>> change is correct.
>>>>
>>>> I'll file a bug against the specs.
>>>>
>>>> In the meanwhile, this is the expected behavior also in the CTS tests,
>>>> which is also confirmed with the nVIDIA blob.
>>>>
>>>>>> This patch amends our implementation to account for this restriction.
>>>>>>
>>>>>> In the process of doing this, I also noticed that we would attempt
>>>>>> to check aliasing requirements for record variables (including the test
>>>>>> for the numerical type) which is not allowed, instead, we should be
>>>>>> producing a linker error as soon as we see any attempt to do location
>>>>>> aliasing on non-numerical variables. For the particular case of structs,
>>>>>> we were producing a linker error in this case, but only because we
>>>>>> assumed that struct fields use all components in each location, so
>>>>>> any attempt to alias locations consumed by struct fields would produce
>>>>>> a link error due to component aliasing, which is not accurate of the
>>>>>> actual problem. This patch would make it produce an error for attempting
>>>>>> to alias a non-numerical variable instead, which is always accurate.
>>>>>
>>>>> None of this should be needed at all. It is an error to use
>>>>> location/component layouts on struct members.
>>>>>
>>>>> "It is a compile-time error to use a location qualifier on a member of a
>>>>> structure."
>>>>>
>>>>> "It is a compile-time error to use *component* without also specifying
>>>>> *location*"
>>>>>
>>>>> So depending on the component aliasing check is sufficient. If you
>>>>> really want to add a better error message then see my comments below.
>>>>>
>>>>>
>>>>>> v2:
>>>>>>      - Do not assert if we see invalid numerical types. These come
>>>>>>        straight from shader code, so we should produce linker errors if
>>>>>>        shaders attempt to do location aliasing on variables that are not
>>>>>>        numerical such as records.
>>>>>>      - While we are at it, improve error reporting for the case of
>>>>>>        numerical type mismatch to include the shader stage.
>>>>>>
>>>>>> v3:
>>>>>>      - Allow location aliasing of images and samplers. If we get these
>>>>>>        it means bindless support is active and they should be handled
>>>>>>        as 64-bit integers (Ilia)
>>>>>>      - Make sure we produce link errors for any non-numerical type
>>>>>>        for which we attempt location aliasing, not just structs.
>>>>>>
>>>>>> v4:
>>>>>>      - Rebased with minor fixes (Andres).
>>>>>>      - Added fixing tag to the commit log (Andres).
>>>>>>
>>>>>> Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
>>>>>> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
>>>>>> Signed-off-by: Andres Gomez <agomez@igalia.com>
>>>>>> ---
>>>>>>     src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
>>>>>>     1 file changed, 46 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
>>>>>> index 3969c0120b3..3f41832ac93 100644
>>>>>> --- a/src/compiler/glsl/link_varyings.cpp
>>>>>> +++ b/src/compiler/glsl/link_varyings.cpp
>>>>>> @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
>>>>>>     
>>>>>>     struct explicit_location_info {
>>>>>>        ir_variable *var;
>>>>>> -   unsigned numerical_type;
>>>>>> +   int numerical_type;
>>>>>>        unsigned interpolation;
>>>>>>        bool centroid;
>>>>>>        bool sample;
>>>>>>        bool patch;
>>>>>>     };
>>>>>>     
>>>>>> -static inline unsigned
>>>>>> -get_numerical_type(const glsl_type *type)
>>>>>> +static inline int
>>>>>> +get_numerical_sized_type(const glsl_type *type)
>>>>>>     {
>>>>>>        /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
>>>>>>         * (Location aliasing):
>>>>>> @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
>>>>>>         *    "Further, when location aliasing, the aliases sharing the location
>>>>>>         *     must have the same underlying numerical type  (floating-point or
>>>>>>         *     integer)
>>>>>> +    *
>>>>>> +    * Khronos has further clarified that this also requires the underlying
>>>>>> +    * types to have the same width, so we can't put a float and a double
>>>>>> +    * in the same location slot for example. Future versions of the spec will
>>>>>> +    * be corrected to make this clear.
>>>>>> +    *
>>>>>> +    * Notice that we allow location aliasing for bindless image/samplers too
>>>>>> +    * since these are defined as 64-bit integers.
>>>>>>         */
>>>>>> -   if (type->is_float() || type->is_double())
>>>>>> +   if (type->is_float())
>>>>>>           return GLSL_TYPE_FLOAT;
>>>>>> -   return GLSL_TYPE_INT;
>>>>>> +   else if (type->is_integer())
>>>>>> +      return GLSL_TYPE_INT;
>>>>>> +   else if (type->is_double())
>>>>>> +      return GLSL_TYPE_DOUBLE;
>>>>>> +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
>>>>>> +      return GLSL_TYPE_INT64;
>>>>>> +
>>>>>> +   return -1; /* Not a numerical type */
>>>>>>     }
>>>>>>     
>>>>>>     static bool
>>>>>> @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>>>>                             gl_shader_stage stage)
>>>>>>     {
>>>>>>        unsigned last_comp;
>>>>>> -   if (type->without_array()->is_record()) {
>>>>>> -      /* The component qualifier can't be used on structs so just treat
>>>>>> -       * all component slots as used.
>>>>>> +   const glsl_type *type_without_array = type->without_array();
>>>>>> +   int numerical_type = get_numerical_sized_type(type_without_array);
>>>>>> +   if (-1 == numerical_type) {
>>>>>> +      /* The component qualifier can't be used on non-numerical types so just
>>>>>> +       * treat all component slots as used. This will also make it so that
>>>>>> +       * any location aliasing attempt on non-numerical types is detected.
>>>>>>            */
>>>>>>           last_comp = 4;
>>>>>>        } else {
>>>>>> -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
>>>>>> -      last_comp = component + type->without_array()->vector_elements * dmul;
>>>>>> +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
>>>>>> +      last_comp = component + type_without_array->vector_elements * dmul;
>>>>>>        }
>>>>>>     
>>>>>>        while (location < location_limit) {
>>>>>> @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>>>>                 &explicit_locations[location][comp];
>>>>>>     
>>>>>>              if (info->var) {
>>>>>> -            /* Component aliasing is not alloed */
>>>>>> -            if (comp >= component && comp < last_comp) {
>>>>>> +            if (-1 == numerical_type || -1 == info->numerical_type) {
>>>>>
>>>>> If you really want a new message for trying to pack with a struct then
>>>>> all you really need here is:
>>>>>
>>>>> if (info->var->type->without_array()->is_record())
>>>>>
>>>>> Then you can skip all this nasty -1 stuff.
>>>>>
>>>>>> +               /* Location aliasing is only allowed for numerical variables */
>>>>>> +               linker_error(prog,
>>>>>> +                            "%s shader has location aliasing for "
>>>>>> +                            "non-numerical variable '%s'. Location %u "
>>>>>> +                            "component %u\n",
>>>>>> +                            _mesa_shader_stage_to_string(stage),
>>>>>> +                            -1 == numerical_type ? var->name : info->var->name,
>>>>>> +                            location, comp);
>>>>>> +               return false;
>>>>>> +            } else if (comp >= component && comp < last_comp) {
>>>>>> +               /* Component aliasing is not allowed */
>>>>>>                    linker_error(prog,
>>>>>>                                 "%s shader has multiple %sputs explicitly "
>>>>>>                                 "assigned to location %d and component %d\n",
>>>>>> @@ -491,12 +519,12 @@ 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->numerical_type !=
>>>>>> -                   get_numerical_type(type->without_array())) {
>>>>>> +               if (info->numerical_type != numerical_type) {
>>>>>>                       linker_error(prog,
>>>>>> -                               "Varyings sharing the same location must "
>>>>>> -                               "have the same underlying numerical type. "
>>>>>> -                               "Location %u component %u\n",
>>>>>> +                               "%s shader has varyings sharing the same "
>>>>>> +                               "location that don't have the same underlying "
>>>>>> +                               "numerical type. Location %u component %u\n",
>>>>>> +                               _mesa_shader_stage_to_string(stage),
>>>>>>                                    location, comp);
>>>>>>                       return false;
>>>>>>                    }
>>>>>> @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>>>>>>                 }
>>>>>>              } else if (comp >= component && comp < last_comp) {
>>>>>>                 info->var = var;
>>>>>> -            info->numerical_type = get_numerical_type(type->without_array());
>>>>>> +            info->numerical_type = numerical_type;
>>>>>>                 info->interpolation = interpolation;
>>>>>>                 info->centroid = centroid;
>>>>>>                 info->sample = sample;
>>>>>>

On Tue, 2019-04-09 at 08:40 -0700, Dylan Baker wrote:
> Hi Andres,
> 
> This doesn't apply cleanly to the 19.0 branch, and I'm not even sure where to
> start resolving the conflicts. If you still want this in 19.0 can you backport
> this and either create an MR against the staging/19.0 branch and mention me, or
> send a patch to the stable list and CC me? (I prefer the MR :))

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/641

The backport is not the cleanest since I had to pull also some other
change but I think it should be safe.

However, the fix is not that important so it could also be dropped.
Take a look to the (additional) changes and feel free to pick or not.

> 
> Thanks,
> Dylan
> 
> Quoting Andres Gomez (2019-02-01 10:05:52)
> > From: Iago Toral Quiroga <itoral@igalia.com>
> > 
> > Regarding location aliasing requirements, the OpenGL spec says:
> > 
> >   "Further, when location aliasing, the aliases sharing the location
> >    must have the same underlying numerical type  (floating-point or
> >    integer)."
> > 
> > Khronos has further clarified that this also requires the underlying
> > types to have the same width, so we can't put a float and a double
> > in the same location slot for example. Future versions of the spec will
> > be corrected to make this clear.
> > 
> > This patch amends our implementation to account for this restriction.
> > 
> > In the process of doing this, I also noticed that we would attempt
> > to check aliasing requirements for record variables (including the test
> > for the numerical type) which is not allowed, instead, we should be
> > producing a linker error as soon as we see any attempt to do location
> > aliasing on non-numerical variables. For the particular case of structs,
> > we were producing a linker error in this case, but only because we
> > assumed that struct fields use all components in each location, so
> > any attempt to alias locations consumed by struct fields would produce
> > a link error due to component aliasing, which is not accurate of the
> > actual problem. This patch would make it produce an error for attempting
> > to alias a non-numerical variable instead, which is always accurate.
> > 
> > v2:
> >   - Do not assert if we see invalid numerical types. These come
> >     straight from shader code, so we should produce linker errors if
> >     shaders attempt to do location aliasing on variables that are not
> >     numerical such as records.
> >   - While we are at it, improve error reporting for the case of
> >     numerical type mismatch to include the shader stage.
> > 
> > v3:
> >   - Allow location aliasing of images and samplers. If we get these
> >     it means bindless support is active and they should be handled
> >     as 64-bit integers (Ilia)
> >   - Make sure we produce link errors for any non-numerical type
> >     for which we attempt location aliasing, not just structs.
> > 
> > v4:
> >   - Rebased with minor fixes (Andres).
> >   - Added fixing tag to the commit log (Andres).
> > 
> > Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing")
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Signed-off-by: Andres Gomez <agomez@igalia.com>
> > ---
> >  src/compiler/glsl/link_varyings.cpp | 64 +++++++++++++++++++++--------
> >  1 file changed, 46 insertions(+), 18 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> > index 3969c0120b3..3f41832ac93 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -424,15 +424,15 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage)
> >  
> >  struct explicit_location_info {
> >     ir_variable *var;
> > -   unsigned numerical_type;
> > +   int numerical_type;
> >     unsigned interpolation;
> >     bool centroid;
> >     bool sample;
> >     bool patch;
> >  };
> >  
> > -static inline unsigned
> > -get_numerical_type(const glsl_type *type)
> > +static inline int
> > +get_numerical_sized_type(const glsl_type *type)
> >  {
> >     /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68,
> >      * (Location aliasing):
> > @@ -440,10 +440,25 @@ get_numerical_type(const glsl_type *type)
> >      *    "Further, when location aliasing, the aliases sharing the location
> >      *     must have the same underlying numerical type  (floating-point or
> >      *     integer)
> > +    *
> > +    * Khronos has further clarified that this also requires the underlying
> > +    * types to have the same width, so we can't put a float and a double
> > +    * in the same location slot for example. Future versions of the spec will
> > +    * be corrected to make this clear.
> > +    *
> > +    * Notice that we allow location aliasing for bindless image/samplers too
> > +    * since these are defined as 64-bit integers.
> >      */
> > -   if (type->is_float() || type->is_double())
> > +   if (type->is_float())
> >        return GLSL_TYPE_FLOAT;
> > -   return GLSL_TYPE_INT;
> > +   else if (type->is_integer())
> > +      return GLSL_TYPE_INT;
> > +   else if (type->is_double())
> > +      return GLSL_TYPE_DOUBLE;
> > +   else if (type->is_integer_64() || type->is_sampler() || type->is_image())
> > +      return GLSL_TYPE_INT64;
> > +
> > +   return -1; /* Not a numerical type */
> >  }
> >  
> >  static bool
> > @@ -461,14 +476,17 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >                          gl_shader_stage stage)
> >  {
> >     unsigned last_comp;
> > -   if (type->without_array()->is_record()) {
> > -      /* The component qualifier can't be used on structs so just treat
> > -       * all component slots as used.
> > +   const glsl_type *type_without_array = type->without_array();
> > +   int numerical_type = get_numerical_sized_type(type_without_array);
> > +   if (-1 == numerical_type) {
> > +      /* The component qualifier can't be used on non-numerical types so just
> > +       * treat all component slots as used. This will also make it so that
> > +       * any location aliasing attempt on non-numerical types is detected.
> >         */
> >        last_comp = 4;
> >     } else {
> > -      unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> > -      last_comp = component + type->without_array()->vector_elements * dmul;
> > +      unsigned dmul = type_without_array->is_64bit() ? 2 : 1;
> > +      last_comp = component + type_without_array->vector_elements * dmul;
> >     }
> >  
> >     while (location < location_limit) {
> > @@ -478,8 +496,18 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >              &explicit_locations[location][comp];
> >  
> >           if (info->var) {
> > -            /* Component aliasing is not alloed */
> > -            if (comp >= component && comp < last_comp) {
> > +            if (-1 == numerical_type || -1 == info->numerical_type) {
> > +               /* Location aliasing is only allowed for numerical variables */
> > +               linker_error(prog,
> > +                            "%s shader has location aliasing for "
> > +                            "non-numerical variable '%s'. Location %u "
> > +                            "component %u\n",
> > +                            _mesa_shader_stage_to_string(stage),
> > +                            -1 == numerical_type ? var->name : info->var->name,
> > +                            location, comp);
> > +               return false;
> > +            } else if (comp >= component && comp < last_comp) {
> > +               /* Component aliasing is not allowed */
> >                 linker_error(prog,
> >                              "%s shader has multiple %sputs explicitly "
> >                              "assigned to location %d and component %d\n",
> > @@ -491,12 +519,12 @@ 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->numerical_type !=
> > -                   get_numerical_type(type->without_array())) {
> > +               if (info->numerical_type != numerical_type) {
> >                    linker_error(prog,
> > -                               "Varyings sharing the same location must "
> > -                               "have the same underlying numerical type. "
> > -                               "Location %u component %u\n",
> > +                               "%s shader has varyings sharing the same "
> > +                               "location that don't have the same underlying "
> > +                               "numerical type. Location %u component %u\n",
> > +                               _mesa_shader_stage_to_string(stage),
> >                                 location, comp);
> >                    return false;
> >                 }
> > @@ -526,7 +554,7 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
> >              }
> >           } else if (comp >= component && comp < last_comp) {
> >              info->var = var;
> > -            info->numerical_type = get_numerical_type(type->without_array());
> > +            info->numerical_type = numerical_type;
> >              info->interpolation = interpolation;
> >              info->centroid = centroid;
> >              info->sample = sample;
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev