From c4545676d7f4e5f898bdc54d5574cd56ca7b9aad Mon Sep 17 00:00:00 2001 From: Iago Toral Quiroga Date: Thu, 19 Oct 2017 13:44:48 +0200 Subject: [PATCH] glsl/linker: fix location aliasing checks for interface variables The existing code was checking the whole interface variable rather than its members, which is not what we want: we want to check aliasing for each member in the interface variable. Surprisingly, there are piglit tests that verify this and were passing due to a bug in the existing code: when we were computing the last component used by an interface variable we would use the 'vector' path and multiply by vector_elements, which is 0 for interface variables. This made the loop that checks for aliasing be a no-op and not add the interface variable to the list of outputs so then we would fail to link when we did not see a matching output for the same input in the next stage. Since the tests expect a linker error to happen, they would pass, but not for the right reason. Unfortunately, the current implementation uses ir_variable instances to keep track of explicit locations. Since we don't have ir_variables instances for individual interface members, we need to have a custom struct with the data we need. This struct has the ir_variable (which for interface members is the whole interface variable), plus the data that we need to validate for each aliased location, for now only the base type, which for interface members we will take from the appropriate field inside the interface variable. Later patches will expand this custom struct so we can also check other requirements for location aliasing, specifically that we have matching interpolation and auxiliary storage, that once again, we will take from the appropriate field members for the interface variables. v2: - Use MAX_VARYING instead of MAX_VARYINGS_INCL_PATCH (Illia) Fixes: KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations Fixes (these were passing before but for incorrect reasons): tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-location-overlap.shader_test tests/spec/arb_enhanced_layouts/linker/block-member-locations/named-block-member-mixed-order-overlap.shader_test Reviewed-by: Timothy Arceri Reviewed-by: Ilia Mirkin --- src/compiler/glsl/link_varyings.cpp | 45 +++++++++++++++++++++-------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp index d394488ab91..1db851b7e9d 100644 --- a/src/compiler/glsl/link_varyings.cpp +++ b/src/compiler/glsl/link_varyings.cpp @@ -403,8 +403,13 @@ compute_variable_location_slot(ir_variable *var, gl_shader_stage stage) return var->data.location - location_start; } +struct explicit_location_info { + ir_variable *var; + unsigned base_type; +}; + static bool -check_location_aliasing(ir_variable *explicit_locations[][4], +check_location_aliasing(struct explicit_location_info explicit_locations[][4], ir_variable *var, unsigned location, unsigned component, @@ -427,7 +432,7 @@ check_location_aliasing(ir_variable *explicit_locations[][4], while (location < location_limit) { unsigned i = component; while (i < last_comp) { - if (explicit_locations[location][i] != NULL) { + if (explicit_locations[location][i].var != NULL) { linker_error(prog, "%s shader has multiple outputs explicitly " "assigned to location %d and component %d\n", @@ -439,9 +444,9 @@ check_location_aliasing(ir_variable *explicit_locations[][4], /* Make sure all component at this location have the same type. */ for (unsigned j = 0; j < 4; j++) { - if (explicit_locations[location][j] && - (explicit_locations[location][j]->type->without_array() - ->base_type != type->without_array()->base_type)) { + if (explicit_locations[location][j].var && + explicit_locations[location][j].base_type != + type->without_array()->base_type) { linker_error(prog, "Varyings sharing the same location must " "have the same underlying numerical type. " @@ -450,7 +455,9 @@ check_location_aliasing(ir_variable *explicit_locations[][4], } } - explicit_locations[location][i] = var; + explicit_locations[location][i].var = var; + explicit_locations[location][i].base_type = + type->without_array()->base_type; i++; /* We need to do some special handling for doubles as dvec3 and @@ -482,8 +489,7 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx, gl_linked_shader *consumer) { glsl_symbol_table parameters; - ir_variable *explicit_locations[MAX_VARYINGS_INCL_PATCH][4] = - { {NULL, NULL} }; + struct explicit_location_info explicit_locations[MAX_VARYING][4] = { 0 }; /* Find all shader outputs in the "producer" stage. */ @@ -514,9 +520,24 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx, return; } - if (!check_location_aliasing(explicit_locations, var, idx, - var->data.location_frac, slot_limit, - type, prog, producer->Stage)) { + if (type->without_array()->is_interface()) { + for (unsigned i = 0; i < type->without_array()->length; i++) { + const glsl_type *field_type = type->fields.structure[i].type; + unsigned field_location = type->fields.structure[i].location - + (type->fields.structure[i].patch ? VARYING_SLOT_PATCH0 : + VARYING_SLOT_VAR0); + if (!check_location_aliasing(explicit_locations, var, + field_location, + 0, field_location + 1, + field_type, prog, + producer->Stage)) { + return; + } + } + } else if (!check_location_aliasing(explicit_locations, var, + idx, var->data.location_frac, + slot_limit, type, prog, + producer->Stage)) { return; } } @@ -581,7 +602,7 @@ cross_validate_outputs_to_inputs(struct gl_context *ctx, return; } - output = explicit_locations[idx][input->data.location_frac]; + output = explicit_locations[idx][input->data.location_frac].var; if (output == NULL || input->data.location != output->data.location) { -- 2.30.2