[Mesa-dev] [PATCH v5 1/6] glsl/linker: location aliasing requires types to have the same width
Andres Gomez
agomez at igalia.com
Fri Mar 29 10:38:34 UTC 2019
>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 <imirkin at alum.mit.edu>
Cc: Timothy Arceri <tarceri at itsqueeze.com>
Cc: Iago Toral Quiroga <itoral at igalia.com>
Signed-off-by: Andres Gomez <agomez at igalia.com>
---
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 352cd7e522b..4d4df8dc2e7 100644
--- a/src/compiler/glsl/link_varyings.cpp
+++ b/src/compiler/glsl/link_varyings.cpp
@@ -424,28 +424,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,
@@ -461,14 +447,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) {
@@ -478,8 +473,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",
@@ -488,27 +497,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;
}
@@ -516,17 +550,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.20.1
More information about the mesa-dev
mailing list