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

[Timothy Arceri]

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>


> +                                            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) {
>