From 75a3dd97aaeb5fea72ab432c8e9f4bd4e50877ed Mon Sep 17 00:00:00 2001 From: Andres Gomez Date: Fri, 29 Mar 2019 12:38:34 +0200 Subject: [PATCH] glsl/linker: location aliasing requires types to have the same width From the OpenGL 4.60.5 spec, section 4.4.1 Input Layout Qualifiers, Page 67, (Location aliasing): " 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." Additionally, we have improved the linker error descriptions. Specifically, when taking structs into account we were producing a linker error because we assumed that all components in each location were used and that would cause component aliasing. This is not accurate of the actual problem. Now, the failure specifies that the underlying numerical type incompatibility is the cause for the failure. Fixes the following piglit test: tests/spec/arb_enhanced_layouts/linker/component-layout/vs-to-fs-width-mismatch-double-float.shader_test 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). v5: - Remove the helper function and check individually for the underlying numerical type and bit width (Timothy). - Implicitly, assume that any non-treated type which is checked for its underlying numerical type is either integer or float and has a defined bit width (Timothy). - Implicitly, assume that structs are the only non-treated non-numerical type (Timothy). - Improve the linker error descriptions and commit log (Andres). Fixes: 13652e7516a ("glsl/linker: Fix type checks for location aliasing") Cc: Ilia Mirkin Cc: Timothy Arceri Cc: Iago Toral Quiroga Signed-off-by: Andres Gomez Reviewed-by: Timothy Arceri --- src/compiler/glsl/link_varyings.cpp | 115 ++++++++++++++++++---------- 1 file changed, 76 insertions(+), 39 deletions(-) diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp index b8534d83076..89a1463c878 100644 --- a/src/compiler/glsl/link_varyings.cpp +++ b/src/compiler/glsl/link_varyings.cpp @@ -441,28 +441,14 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage) struct explicit_location_info { ir_variable *var; - unsigned numerical_type; + bool base_type_is_integer; + unsigned base_type_bit_size; unsigned interpolation; bool centroid; bool sample; bool patch; }; -static inline unsigned -get_numerical_type(const glsl_type *type) -{ - /* From the OpenGL 4.6 spec, section 4.4.1 Input Layout Qualifiers, Page 68, - * (Location aliasing): - * - * "Further, when location aliasing, the aliases sharing the location - * must have the same underlying numerical type (floating-point or - * integer) - */ - if (type->is_float() || type->is_double()) - return GLSL_TYPE_FLOAT; - return GLSL_TYPE_INT; -} - static bool check_location_aliasing(struct explicit_location_info explicit_locations[][4], ir_variable *var, @@ -478,14 +464,23 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4], gl_shader_stage stage) { unsigned last_comp; - if (type->without_array()->is_struct()) { - /* The component qualifier can't be used on structs so just treat - * all component slots as used. + unsigned base_type_bit_size; + const glsl_type *type_without_array = type->without_array(); + const bool base_type_is_integer = + glsl_base_type_is_integer(type_without_array->base_type); + const bool is_struct = type_without_array->is_struct(); + if (is_struct) { + /* structs don't have a defined underlying base type so just treat all + * component slots as used and set the bit size to 0. If there is + * location aliasing, we'll fail anyway later. */ last_comp = 4; + base_type_bit_size = 0; } 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; + base_type_bit_size = + glsl_base_type_get_bit_size(type_without_array->base_type); } while (location < location_limit) { @@ -495,8 +490,22 @@ 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 (info->var->type->without_array()->is_struct() || is_struct) { + /* Structs cannot share location since they are incompatible + * with any other underlying numerical type. + */ + linker_error(prog, + "%s shader has multiple %sputs sharing the " + "same location that don't have the same " + "underlying numerical type. Struct variable '%s', " + "location %u\n", + _mesa_shader_stage_to_string(stage), + var->data.mode == ir_var_shader_in ? "in" : "out", + is_struct ? var->name : info->var->name, + location); + 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", @@ -505,27 +514,52 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4], location, comp); return false; } else { - /* For all other used components we need to have matching - * types, interpolation and auxiliary storage + /* From the OpenGL 4.60.5 spec, section 4.4.1 Input Layout + * Qualifiers, Page 67, (Location aliasing): + * + * " 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." + */ + + /* If the underlying numerical type isn't integer, implicitly + * it will be float or else we would have failed by now. */ - if (info->numerical_type != - get_numerical_type(type->without_array())) { + if (info->base_type_is_integer != base_type_is_integer) { linker_error(prog, - "Varyings sharing the same location must " - "have the same underlying numerical type. " - "Location %u component %u\n", - location, comp); + "%s shader has multiple %sputs sharing the " + "same location that don't have the same " + "underlying numerical type. Location %u " + "component %u.\n", + _mesa_shader_stage_to_string(stage), + var->data.mode == ir_var_shader_in ? + "in" : "out", location, comp); + return false; + } + + if (info->base_type_bit_size != base_type_bit_size) { + linker_error(prog, + "%s shader has multiple %sputs sharing the " + "same location that don't have the same " + "underlying numerical bit size. Location %u " + "component %u.\n", + _mesa_shader_stage_to_string(stage), + var->data.mode == ir_var_shader_in ? + "in" : "out", location, comp); return false; } if (info->interpolation != interpolation) { linker_error(prog, - "%s shader has multiple %sputs at explicit " - "location %u with different interpolation " - "settings\n", + "%s shader has multiple %sputs sharing the " + "same location that don't have the same " + "interpolation qualification. Location %u " + "component %u.\n", _mesa_shader_stage_to_string(stage), var->data.mode == ir_var_shader_in ? - "in" : "out", location); + "in" : "out", location, comp); return false; } @@ -533,17 +567,20 @@ check_location_aliasing(struct explicit_location_info explicit_locations[][4], info->sample != sample || info->patch != patch) { linker_error(prog, - "%s shader has multiple %sputs at explicit " - "location %u with different aux storage\n", + "%s shader has multiple %sputs sharing the " + "same location that don't have the same " + "auxiliary storage qualification. Location %u " + "component %u.\n", _mesa_shader_stage_to_string(stage), var->data.mode == ir_var_shader_in ? - "in" : "out", location); + "in" : "out", location, comp); return false; } } } else if (comp >= component && comp < last_comp) { info->var = var; - info->numerical_type = get_numerical_type(type->without_array()); + info->base_type_is_integer = base_type_is_integer; + info->base_type_bit_size = base_type_bit_size; info->interpolation = interpolation; info->centroid = centroid; info->sample = sample; -- 2.30.2