On Thu, 2017-10-19 at 12:37 -0400, Ilia Mirkin wrote: > Will this work with SSO shaders? Presumably the validation still has > to happen, but I don't think cross_validate_outputs_to_inputs() will > end up getting called.
The piglit tests I mention use SSO so it seems to be working for this. See for example: tests/spec/arb_enhanced_layouts/linker/block-member-locations/named- block-member-location-overlap.shader_test Iago > On Thu, Oct 19, 2017 at 12:31 PM, Iago Toral Quiroga > <[email protected]> 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 }; > > > > /* 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, > > + 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; > > } > > } > > @@ -574,7 +595,7 @@ cross_validate_outputs_to_inputs(struct > > gl_context *ctx, > > unsigned slot_limit = idx + num_elements; > > > > while (idx < slot_limit) { > > - 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.11.0 > > > > _______________________________________________ > > mesa-dev mailing list > > [email protected] > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
