Re: [Mesa-dev] [PATCH 2/2] glsl/linker: location aliasing requires types to have the same width

[Iago Toral]

On Fri, 2017-11-03 at 12:01 -0400, Ilia Mirkin wrote:
> On Fri, Nov 3, 2017 at 6:56 AM, Iago Toral Quiroga  > wrote:
> > 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 a record variable.
> 
> Is there a piglit for this? (Which fails without this patch?)
> 
> Oh, and you hit it because you stuck the unreachable in there?

No, there aren't any tests that catch this, I can add one though.

> 
> > ---
> >  src/compiler/glsl/link_varyings.cpp | 41
> > ++---
> >  1 file changed, 34 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/link_varyings.cpp
> > b/src/compiler/glsl/link_varyings.cpp
> > index af938611f4..1d17deaffe 100644
> > --- a/src/compiler/glsl/link_varyings.cpp
> > +++ b/src/compiler/glsl/link_varyings.cpp
> > @@ -413,7 +413,7 @@ struct explicit_location_info {
> >  };
> > 
> >  static inline unsigned
> > -get_numerical_type(const glsl_type *type)
> > +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):
> > @@ -421,10 +421,22 @@ 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.
> >  */
> > -   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())
> > +  return GLSL_TYPE_INT64;
> 
> How does a (bindless) sampler/image come across? Do they come out as
> ->is_integer_64? (I don't think they do.)

Actually, this should not have an unreachable(). The types we see here
are provided by the user, so we should not assert on them, we should
handle them as linker errors if they are not numerical (and that should
also handle the case of structs without having to special-case them
below). I'll send a v2.

Iago

> > +
> > +   unreachable("Type is not numerical");
> >  }
> > 
> >  static bool
> > @@ -442,14 +454,17 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> >  gl_shader_stage stage)
> >  {
> > unsigned last_comp;
> > +   bool is_record;
> > if (type->without_array()->is_record()) {
> >    /* The component qualifier can't be used on structs so just
> > treat
> > * all component slots as used.
> > */
> >    last_comp = 4;
> > +  is_record = true;
> > } else {
> >    unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
> >    last_comp = component + type->without_array()-
> > >vector_elements * dmul;
> > +  is_record = false;
> > }
> > 
> > while (location < location_limit) {
> > @@ -459,8 +474,17 @@ check_location_aliasing(struct
> > explicit_location_info explicit_locations[][4],
> >  _locations[location][comp];
> > 
> >   if (info->var) {
> > -/* Component aliasing is not alloed */
> > -if (comp >= component && comp < last_comp) {
> > +if (is_record) {
> > +   /* Disallow location aliasing for record variables
> > */
> > +   linker_error(prog,
> > +"%s shader uses explicit location on
> > record "
> > +"variable %s that generates location
> > aliasing "
> > +"for location %u, component %u\n",
> > +_mesa_shader_stage_to_string(stage),
> > +var->name, location, comp);
> > +  

Re: [Mesa-dev] [PATCH 2/2] glsl/linker: location aliasing requires types to have the same width

[Ilia Mirkin]

On Fri, Nov 3, 2017 at 6:56 AM, Iago Toral Quiroga  wrote:
> 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 a record variable.

Is there a piglit for this? (Which fails without this patch?)

Oh, and you hit it because you stuck the unreachable in there?

> ---
>  src/compiler/glsl/link_varyings.cpp | 41 
> ++---
>  1 file changed, 34 insertions(+), 7 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp 
> b/src/compiler/glsl/link_varyings.cpp
> index af938611f4..1d17deaffe 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -413,7 +413,7 @@ struct explicit_location_info {
>  };
>
>  static inline unsigned
> -get_numerical_type(const glsl_type *type)
> +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):
> @@ -421,10 +421,22 @@ 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.
>  */
> -   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())
> +  return GLSL_TYPE_INT64;

How does a (bindless) sampler/image come across? Do they come out as
->is_integer_64? (I don't think they do.)

> +
> +   unreachable("Type is not numerical");
>  }
>
>  static bool
> @@ -442,14 +454,17 @@ check_location_aliasing(struct explicit_location_info 
> explicit_locations[][4],
>  gl_shader_stage stage)
>  {
> unsigned last_comp;
> +   bool is_record;
> if (type->without_array()->is_record()) {
>/* The component qualifier can't be used on structs so just treat
> * all component slots as used.
> */
>last_comp = 4;
> +  is_record = true;
> } else {
>unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
>last_comp = component + type->without_array()->vector_elements * dmul;
> +  is_record = false;
> }
>
> while (location < location_limit) {
> @@ -459,8 +474,17 @@ check_location_aliasing(struct explicit_location_info 
> explicit_locations[][4],
>  _locations[location][comp];
>
>   if (info->var) {
> -/* Component aliasing is not alloed */
> -if (comp >= component && comp < last_comp) {
> +if (is_record) {
> +   /* Disallow location aliasing for record variables */
> +   linker_error(prog,
> +"%s shader uses explicit location on record "
> +"variable %s that generates location aliasing "
> +"for location %u, component %u\n",
> +_mesa_shader_stage_to_string(stage),
> +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 outputs explicitly "
>  "assigned to location %d and component %d\n",
> @@ -472,7 +496,7 @@ check_location_aliasing(struct explicit_location_info 
> explicit_locations[][4],
>  * types, interpolation and auxiliary storage
>  */
> if (info->numerical_type !=
> -   get_numerical_type(type->without_array())) {
> +   get_numerical_sized_type(type->without_array())) {
> 

[Mesa-dev] [PATCH 2/2] glsl/linker: location aliasing requires types to have the same width

[Iago Toral Quiroga]

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 a record variable.
---
 src/compiler/glsl/link_varyings.cpp | 41 ++---
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/src/compiler/glsl/link_varyings.cpp 
b/src/compiler/glsl/link_varyings.cpp
index af938611f4..1d17deaffe 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -413,7 +413,7 @@ struct explicit_location_info {
 };
 
 static inline unsigned
-get_numerical_type(const glsl_type *type)
+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):
@@ -421,10 +421,22 @@ 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.
 */
-   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())
+  return GLSL_TYPE_INT64;
+
+   unreachable("Type is not numerical");
 }
 
 static bool
@@ -442,14 +454,17 @@ check_location_aliasing(struct explicit_location_info 
explicit_locations[][4],
 gl_shader_stage stage)
 {
unsigned last_comp;
+   bool is_record;
if (type->without_array()->is_record()) {
   /* The component qualifier can't be used on structs so just treat
* all component slots as used.
*/
   last_comp = 4;
+  is_record = true;
} else {
   unsigned dmul = type->without_array()->is_64bit() ? 2 : 1;
   last_comp = component + type->without_array()->vector_elements * dmul;
+  is_record = false;
}
 
while (location < location_limit) {
@@ -459,8 +474,17 @@ check_location_aliasing(struct explicit_location_info 
explicit_locations[][4],
 _locations[location][comp];
 
  if (info->var) {
-/* Component aliasing is not alloed */
-if (comp >= component && comp < last_comp) {
+if (is_record) {
+   /* Disallow location aliasing for record variables */
+   linker_error(prog,
+"%s shader uses explicit location on record "
+"variable %s that generates location aliasing "
+"for location %u, component %u\n",
+_mesa_shader_stage_to_string(stage),
+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 outputs explicitly "
 "assigned to location %d and component %d\n",
@@ -472,7 +496,7 @@ check_location_aliasing(struct explicit_location_info 
explicit_locations[][4],
 * types, interpolation and auxiliary storage
 */
if (info->numerical_type !=
-   get_numerical_type(type->without_array())) {
+   get_numerical_sized_type(type->without_array())) {
   linker_error(prog,
"Varyings sharing the same location must "
"have the same underlying numerical type. "
@@ -502,7 +526,10 @@ 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());
+if (!is_record) {
+