[Mesa-dev] [PATCH v2] glsl: disallow mixed varying types within a location

[Timothy Arceri]

At the risk of sounding like a broken record. Why not just 
update/re-factor cross_validate_outputs_to_inputs()?

If there is a good reason not to have these checks in a single location 
that's fine but you haven't answered the question yet.

On 10/09/17 03:09, Ilia Mirkin wrote:
> The enhanced layouts spec has all kinds of restrictions about what can
> and cannot be mixed in a location. Integer/float(/presumably double)
> can't occupy a single location, interpolation has to be the same, as
> well as auxiliary storage (including patch!).
> 
> The implication of this is ... don't specify explicit locations/components
> if you want better packing, since the auto-packer doesn't care at all
> about most of these restrictions. Sad. (Auto-packer, of course, also
> can't always do its magic, e.g. SSO.)
> 
> This fixes:
> 
> KHR-GL45.enhanced_layouts.varying_location_aliasing_with_mixed_types
> KHR-GL45.enhanced_layouts.varying_location_aliasing_with_mixed_interpolation
> KHR-GL45.enhanced_layouts.varying_location_aliasing_with_mixed_auxiliary_storage
> 
> See https://github.com/KhronosGroup/OpenGL-API/issues/13 for some more
> info, where I asked about patch vs non-patch locations.
> 
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
> 
> v1 -> v2: move to link_varyings.cpp, per tarceri
> 
>   src/compiler/glsl/link_varyings.cpp | 90 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 90 insertions(+)
> 
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index 528506fd0eb..0aab083010e 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -2438,6 +2438,87 @@ check_against_input_limit(struct gl_context *ctx,
>      return true;
>   }
>   
> +static void
> +check_location_mixing(struct gl_shader_program *prog,
> +                      struct gl_linked_shader *shader)
> +{
> +   struct data_type {
> +      // 0: unused, 1: float, 2: int, 3: 64-bit
> +      unsigned var_type:2;
> +      unsigned interpolation:2;
> +      bool centroid:1;
> +      bool sample:1;
> +      bool patch:1;
> +   } data_types[2][MAX_VARYING] = {};
> +
> +   foreach_in_list(ir_instruction, node, shader->ir) {
> +      ir_variable *var = node->as_variable();
> +      if (!var || !var->data.explicit_location)
> +         continue;
> +
> +      if (var->data.mode != ir_var_shader_in &&
> +          var->data.mode != ir_var_shader_out)
> +         continue;
> +
> +      bool output = var->data.mode == ir_var_shader_out;
> +      int var_slot;
> +      if (!output && shader->Stage == MESA_SHADER_VERTEX) {
> +         var_slot = var->data.location - VERT_ATTRIB_GENERIC0;
> +         if (var_slot >= VERT_ATTRIB_GENERIC_MAX)
> +            continue;
> +      } else if (var->data.patch) {
> +         var_slot = var->data.location - VARYING_SLOT_PATCH0;
> +         if (var_slot >= MAX_VARYING)
> +            continue;
> +      } else {
> +         var_slot = var->data.location - VARYING_SLOT_VAR0;
> +         if (var_slot >= MAX_VARYING)
> +            continue;
> +      }
> +
> +      if (var_slot < 0)
> +         continue;
> +
> +      const glsl_type *type = get_varying_type(var, shader->Stage);
> +      const glsl_type *type_without_array = type->without_array();
> +      unsigned num_elements = type->count_attribute_slots(false);
> +      unsigned var_type;
> +      if (glsl_base_type_is_64bit(type_without_array->base_type))
> +         var_type = 3;
> +      else if (glsl_base_type_is_integer(type_without_array->base_type))
> +         var_type = 2;
> +      else
> +         var_type = 1;
> +
> +      const char *var_dir = output ? "out" : "in";
> +
> +      for (unsigned i = 0; i < num_elements; i++, var_slot++) {
> +         data_type& existing = data_types[output][var_slot];
> +
> +         if (existing.var_type) {
> +            if (existing.var_type != var_type)
> +               linker_error(prog, "Mismatching %s varying types at location=%d",
> +                            var_dir, var_slot);
> +            if (existing.interpolation != var->data.interpolation)
> +               linker_error(prog, "Mismatching %s varying interpolation at location=%d",
> +                            var_dir, var_slot);
> +            if (existing.centroid != var->data.centroid ||
> +                existing.sample != var->data.sample ||
> +                existing.patch != var->data.patch)
> +               linker_error(prog, "Mismatching %s varying auxiliary storage at location=%d",
> +                            var_dir, var_slot);
> +         } else {
> +            existing.var_type = var_type;
> +            existing.interpolation = var->data.interpolation;
> +            existing.centroid = var->data.centroid;
> +            existing.sample = var->data.sample;
> +            existing.patch = var->data.patch;
> +         }
> +      }
> +   }
> +}
> +
> +
>   bool
>   link_varyings(struct gl_shader_program *prog, unsigned first, unsigned last,
>                 struct gl_context *ctx, void *mem_ctx)
> @@ -2447,6 +2528,15 @@ link_varyings(struct gl_shader_program *prog, unsigned first, unsigned last,
>      char **varying_names = NULL;
>      tfeedback_decl *tfeedback_decls = NULL;
>   
> +   /* Ensure that no illegal type/interp/storage mixing exists within
> +    * explicitly-specified varying/attribute locations.
> +    */
> +   for (int i = MESA_SHADER_FRAGMENT - 1; i >= 0; i--)
> +      if (prog->_LinkedShaders[i] != NULL)
> +         check_location_mixing(prog, prog->_LinkedShaders[i]);
> +   if (!prog->data->LinkStatus)
> +      return false;
> +
>      /* From the ARB_enhanced_layouts spec:
>       *
>       *    "If the shader used to record output variables for transform feedback
>