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

[Timothy Arceri]

On 2/2/19 5:05 am, Andres Gomez wrote:
> From: Iago Toral Quiroga <itoral at 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 at alum.mit.edu>
> Signed-off-by: Andres Gomez <agomez at 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;
>