[Mesa-dev] [PATCH 2/4] glsl/linker: fix location aliasing checks for interface variables

[Ilia Mirkin]

On Thu, Oct 19, 2017 at 10:18 PM, Timothy Arceri <tarceri at itsqueeze.com> 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 <tarceri at itsqueeze.com>

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