On Thu, Oct 19, 2017 at 10:18 PM, Timothy Arceri <[email protected]> wrote: > > > On 20/10/17 03:31, Iago Toral Quiroga wrote: >> >> 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. >> >> Fixes: >> KHR-GL45.enhanced_layouts.varying_block_automatic_member_locations >> >> Fixes: >> >> tests/spec/arb_separate_shader_objects/execution/layout-location-named-block.shader_test >> -auto -fbo >> >> 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 >> >> --- >> 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 687bd50e52..5dc2704321 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,8 @@ 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_VARYINGS_INCL_PATCH][4] = >> + { 0 }; > > > I think this need to be something like: > > {{{ 0 }, { 0 }}}; > > ??? > > >> /* Find all shader outputs in the "producer" stage. >> */ >> @@ -514,9 +521,23 @@ 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 - >> VARYING_SLOT_VAR0; >> + if (!check_location_aliasing(explicit_locations, var, >> + field_location, >> + 0, field_location + 1, > > > The spec says interface members can have component qualifiers, however it > seems this is an existing bug. > > With the initialization of explicit_locations fixed above. 1-2 are: > > Reviewed-by: Timothy Arceri <[email protected]>
Like I said, pretty sure this patch won't work for SSO shaders (with one stage per program). The one I wrote should work though. -ilia _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
