[Mesa-dev] [PATCH] glsl: disallow mixed varying types within a location
Timothy Arceri
tarceri at itsqueeze.com
Wed Sep 6 01:54:20 UTC 2017
On 06/09/17 11:23, 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.
There are still use cases such as SSO, tessellation shaders and varyings
used by interpolateAt (although we just enable the enhanced layout
packing rules by default for those anyway) were we cannot use the
auto-packer.
As far as the patch goes this should really be in link_varyings.cpp
rather than linker.cpp, also there is already related validation code in
cross_validate_outputs_to_inputs() any reason for not just modifying the
code there?
>
> 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>
> ---
> src/compiler/glsl/link_varyings.cpp | 23 --------
> src/compiler/glsl/linker.cpp | 115 ++++++++++++++++++++++++++++++++++++
> src/compiler/glsl/linker.h | 4 ++
> 3 files changed, 119 insertions(+), 23 deletions(-)
>
> diff --git a/src/compiler/glsl/link_varyings.cpp b/src/compiler/glsl/link_varyings.cpp
> index 528506fd0eb..20187166203 100644
> --- a/src/compiler/glsl/link_varyings.cpp
> +++ b/src/compiler/glsl/link_varyings.cpp
> @@ -40,29 +40,6 @@
> #include "program.h"
>
>
> -/**
> - * Get the varying type stripped of the outermost array if we're processing
> - * a stage whose varyings are arrays indexed by a vertex number (such as
> - * geometry shader inputs).
> - */
> -static const glsl_type *
> -get_varying_type(const ir_variable *var, gl_shader_stage stage)
> -{
> - const glsl_type *type = var->type;
> -
> - if (!var->data.patch &&
> - ((var->data.mode == ir_var_shader_out &&
> - stage == MESA_SHADER_TESS_CTRL) ||
> - (var->data.mode == ir_var_shader_in &&
> - (stage == MESA_SHADER_TESS_CTRL || stage == MESA_SHADER_TESS_EVAL ||
> - stage == MESA_SHADER_GEOMETRY)))) {
> - assert(type->is_array());
> - type = type->fields.array;
> - }
> -
> - return type;
> -}
> -
> static void
> create_xfb_varying_names(void *mem_ctx, const glsl_type *t, char **name,
> size_t name_length, unsigned *count,
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index 5c3f1d12bbc..3afe5b52a91 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -2155,6 +2155,109 @@ link_cs_input_layout_qualifiers(struct gl_shader_program *prog,
> }
> }
>
> +/**
> + * Get the varying type stripped of the outermost array if we're processing
> + * a stage whose varyings are arrays indexed by a vertex number (such as
> + * geometry shader inputs).
> + */
> +const glsl_type *
> +get_varying_type(const ir_variable *var, gl_shader_stage stage)
> +{
> + const glsl_type *type = var->type;
> +
> + if (!var->data.patch &&
> + ((var->data.mode == ir_var_shader_out &&
> + stage == MESA_SHADER_TESS_CTRL) ||
> + (var->data.mode == ir_var_shader_in &&
> + (stage == MESA_SHADER_TESS_CTRL || stage == MESA_SHADER_TESS_EVAL ||
> + stage == MESA_SHADER_GEOMETRY)))) {
> + assert(type->is_array());
> + type = type->fields.array;
> + }
> +
> + return type;
> +}
> +
> +static void
> +validate_intrastage_location_types(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;
> + }
> + }
> + }
> +}
> +
>
> /**
> * Combine a group of shaders for a single stage to generate a linked shader
> @@ -4941,6 +5044,18 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> lower_named_interface_blocks(mem_ctx, prog->_LinkedShaders[i]);
> }
>
> + /* Check that each explicitly-assigned location doesn't mix
> + * types/interpolation/aux storage qualifiers. This must be done after
> + * lowering named interface blocks.
> + */
> + for (unsigned int i = 0; i < MESA_SHADER_STAGES; i++) {
> + if (prog->_LinkedShaders[i] == NULL)
> + continue;
> + validate_intrastage_location_types(prog, prog->_LinkedShaders[i]);
> + if (!prog->data->LinkStatus)
> + goto done;
> + }
> +
> /* Implement the GLSL 1.30+ rule for discard vs infinite loops Do
> * it before optimization because we want most of the checks to get
> * dropped thanks to constant propagation.
> diff --git a/src/compiler/glsl/linker.h b/src/compiler/glsl/linker.h
> index 5cec121e634..6d6e8edd487 100644
> --- a/src/compiler/glsl/linker.h
> +++ b/src/compiler/glsl/linker.h
> @@ -92,6 +92,10 @@ link_intrastage_shaders(void *mem_ctx,
> unsigned num_shaders,
> bool allow_missing_main);
>
> +const glsl_type *
> +get_varying_type(const ir_variable *var, gl_shader_stage stage);
> +
> +
> /**
> * Class for processing all of the leaf fields of a variable that corresponds
> * to a program resource.
>
More information about the mesa-dev
mailing list