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

[Ilia Mirkin]

So are we saying that you can't have explicit components on a bindless
sampler/image varying, which are defined as 64-bit integer types?

On Mon, Nov 6, 2017 at 7:22 AM, Iago Toral Quiroga <itoral at igalia.com> 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 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 (this includes records, images, etc).
>   - While we are at it, improve error reporting for the case of
>     numerical type mismatch to include the shader stage.
> ---
>  src/compiler/glsl/link_varyings.cpp | 52 +++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index 1a9894baab..f284134c90 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -405,15 +405,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):
> @@ -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;
> +   else
> +      return -1; /* Not a numerical type */
>  }
>
>  static bool
> @@ -442,7 +454,8 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>                          gl_shader_stage stage)
>  {
>     unsigned last_comp;
> -   if (type->without_array()->is_record()) {
> +   const glsl_type *type_without_array = type->without_array();
> +   if (type_without_array->is_record()) {
>        /* The component qualifier can't be used on structs so just treat
>         * all component slots as used.
>         */
> @@ -458,9 +471,20 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4],
>           struct explicit_location_info *info =
>              &explicit_locations[location][comp];
>
> +         int numerical_type = get_numerical_sized_type(type_without_array);
> +
>           if (info->var) {
> -            /* Component aliasing is not alloed */
> -            if (comp >= component && comp < last_comp) {
> +            if (numerical_type == -1) {
> +               /* 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),
> +                            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",
> @@ -471,12 +495,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;
>                 }
> @@ -502,7 +526,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.11.0
>