[Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables
Timothy Arceri
tarceri at itsqueeze.com
Fri Oct 20 10:56:00 UTC 2017
On 20/10/17 16:54, Iago Toral wrote:
> On Fri, 2017-10-20 at 13:18 +1100, Timothy Arceri 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 }}};
>>
>> ???
>
> I didn't do that because it doesn't seem to be necessary after
> inspection.
>
> Since then, I have also seen this:
> https://stackoverflow.com/questions/15520880/initializing-entire-2d-arr
> ay-with-one-value
>
> which explains how this works and pretty much comes to say that
> initializing to { 0 } has the same effect.
>
>>
>>>
>>> /* 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.
>
> Yes, I mentioned that in the cover letter.
>
>> With the initialization of explicit_locations fixed above. 1-2 are:
>
> I think it is not necessary by my reply above, but I am okay with
> changing it if you insist, just let me know if that is the case :)
I'm not an expert on the C spec. If you are sure its ok and there are no
compiler/valgrind warnings I won't force you to change it :)
>
>> Reviewed-by: Timothy Arceri <tarceri at itsqueeze.com>
>>
>>
>>> + 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)
>>> {
>>>
>>
>>
More information about the mesa-dev
mailing list