[Mesa-dev] [PATCH v3] glsl/linker: location aliasing requires types to have the same width
Ilia Mirkin
imirkin at alum.mit.edu
Thu Nov 9 13:41:12 UTC 2017
On Tue, Nov 7, 2017 at 4:50 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 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.
> ---
> 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 1a9894baab..e0d757eaaf 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,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
> @@ -442,14 +457,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 (numerical_type == -1) {
> + /* 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.
> */
But you don't really prevent this here...
layout(component=2) out S foo;
won't run into problems that I can see (at least not due to this
code). Only if something else ends up in that location for the other
comps. Is something else preventing the above?
> 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) {
> @@ -459,8 +477,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 (numerical_type == -1 || info->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),
> + numerical_type == -1 ? 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 outputs explicitly "
> "assigned to location %d and component %d\n",
> @@ -471,12 +499,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 +530,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
>
More information about the mesa-dev
mailing list