Man... linkers are the most complex pieces of software. :( I only have
one significant comment (the second below) and three nits / questions.
With at least the first one resolved, this patch is
Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
On 10/30/2013 04:33 PM, Paul Berry wrote:
> Previously, when doing intrastage and interstage interface block
> linking, we only checked the interface type; this prevented us from
> catching some link errors.
>
> We now check the following additional constraints:
>
> - For intrastage linking, the presence/absence of interface names must
> match.
>
> - For shader ins/outs, the interface names themselves must match when
> doing intrastage linking (note: it's not clear from the spec whether
> this is necessary, but Mesa's implementation currently relies on
> it).
>
> - Array vs. nonarray must be consistent, taking into account the
> special rules for vertex-geometry linkage.
>
> - Array sizes must be consistent (exception: during intrastage
> linking, an unsized array matches a sized array).
>
> Note: validate_interstage_interface_blocks currently handles both
> uniforms and in/out variables. As a result, if all three shader types
> are present (VS, GS, and FS), and a uniform interface block is
> mentioned in the VS and FS but not the GS, it won't be validated. I
> plan to address this in later patches.
>
> Fixes the following piglit tests in spec/glsl-1.50/linker:
> - interface-blocks-vs-fs-array-size-mismatch
> - interface-vs-array-to-fs-unnamed
> - interface-vs-unnamed-to-fs-array
> - intrastage-interface-unnamed-array
> ---
> src/glsl/link_interface_blocks.cpp | 280
> ++++++++++++++++++++++++++++++++++---
> 1 file changed, 260 insertions(+), 20 deletions(-)
>
> diff --git a/src/glsl/link_interface_blocks.cpp
> b/src/glsl/link_interface_blocks.cpp
> index 4f1c9d3..58c9538 100644
> --- a/src/glsl/link_interface_blocks.cpp
> +++ b/src/glsl/link_interface_blocks.cpp
> @@ -30,13 +30,219 @@
> #include "glsl_symbol_table.h"
> #include "linker.h"
> #include "main/macros.h"
> +#include "program/hash_table.h"
> +
> +
> +namespace {
> +
> +/**
> + * Information about a single interface block definition that we need to keep
> + * track of in order to check linkage rules.
> + *
> + * Note: this class is expected to be short lived, so it doesn't make copies
> + * of the strings it references; it simply borrows the pointers from the
> + * ir_variable class.
> + */
> +struct interface_block_definition
> +{
> + /**
> + * Extract an interface block definition from an ir_variable that
> + * represents either the interface instance (for named interfaces), or a
> + * member of the interface (for unnamed interfaces).
> + */
> + explicit interface_block_definition(const ir_variable *var)
> + : type(var->get_interface_type()),
> + instance_name(NULL),
> + array_size(-1)
> + {
> + if (var->is_interface_instance()) {
> + instance_name = var->name;
> + if (var->type->is_array())
> + array_size = var->type->length;
> + }
> + }
> +
> + /**
> + * Interface block type
> + */
> + const glsl_type *type;
> +
> + /**
> + * For a named interface block, the instance name. Otherwise NULL.
> + */
> + const char *instance_name;
> +
> + /**
> + * For an interface block array, the array size (or 0 if unsized).
> + * Otherwise -1.
> + */
> + int array_size;
> +};
> +
> +
> +/**
> + * Check if two interfaces match, according to intrastage interface matching
> + * rules. If they do, and the first interface uses an unsized array, it will
> + * be updated to reflect the array size declared in the second interface.
> + */
> +bool
> +intrastage_match(interface_block_definition *a,
> + const interface_block_definition *b,
> + ir_variable_mode mode)
> +{
> + /* Types must match. */
> + if (a->type != b->type)
> + return false;
> +
> + /* Presence/absence of interface names must match. */
> + if ((a->instance_name == NULL) != (b->instance_name == NULL))
> + return false;
> +
> + /* For uniforms, instance names need not match. For shader ins/outs,
> + * it's not clear from the spec whether they need to match, but
> + * Mesa's implementation relies on them matching.
> + */
This is because the interstage matching is done using the name of the
block instead of the name instance, right? Oof.
> + if (a->instance_name != NULL && mode != ir_var_uniform &&
> + strcmp(a->instance_name, b->instance_name) != 0) {
> + return false;
> + }
> +
> + /* Array vs. nonarray must be consistent, and sizes must be
> + * consistent, with the exception that unsized arrays match sized
> + * arrays.
> + */
> + if (a->array_size == -1) {
> + if (b->array_size != -1)
> + return false;
I think some of the logic here would be easier to follow if array /
non-array mixing were filtered by:
if ((a->array_size == -1) != (b->array_size == -1))
return false;
The rest then becomes
if (b->array_size != 0) {
if (a->array_size == 0)
a->array_size = b->array_size;
else if (a->array_size != b->array_size)
return false;
}
return true;
Right?
> + } else if (a->array_size == 0) {
> + if (b->array_size == -1)
> + return false;
> + else if (b->array_size != 0) {
> + /* An unsized array matches a sized array. Record the size to
> + * match against future declarations.
> + */
> + a->array_size = b->array_size;
> + }
> + } else {
> + /* An unsized array matches a sized array. */
> + if (b->array_size != 0 && a->array_size != b->array_size)
> + return false;
> + }
> + return true;
> +}
> +
> +
> +/**
> + * Check if two interfaces match, according to interstage (in/out) interface
> + * matching rules.
> + *
> + * If \c extra_array_level is true, then vertex-to-geometry shader matching
> + * rules are enforced (i.e. a successful match requires the consumer
> interface
> + * to be an array and the producer interface to be a non-array).
> + */
> +bool
> +interstage_match(const interface_block_definition *producer,
> + const interface_block_definition *consumer,
> + bool extra_array_level)
> +{
> + /* Unsized arrays should not occur during interstage linking. They
> + * should have all been assigned a size by link_intrastage_shaders.
> + */
> + assert(consumer->array_size != 0);
> + assert(producer->array_size != 0);
> +
> + /* Types must match. */
> + if (consumer->type != producer->type)
> + return false;
> + if (extra_array_level) {
> + /* Consumer must be an array, and producer must not. */
> + if (consumer->array_size == -1)
> + return false;
> + if (producer->array_size != -1)
> + return false;
> + } else {
> + /* Array vs. nonarray must be consistent, and sizes must be consistent.
> + * Since unsized arrays have been ruled out, we can check this by just
> + * making sure the sizes are equal.
> + */
> + if (consumer->array_size != producer->array_size)
> + return false;
> + }
> + return true;
> +}
> +
> +
> +/**
> + * This class keeps track of a mapping from an interface block name to the
> + * necessary information about that interface block to determine whether to
> + * generate a link error.
> + *
> + * Note: this class is expected to be short lived, so it doesn't make copies
> + * of the strings it references; it simply borrows the pointers from the
> + * ir_variable class.
> + */
> +class interface_block_definitions
> +{
> +public:
> + interface_block_definitions()
> + : mem_ctx(ralloc_context(NULL)),
> + ht(hash_table_ctor(0, hash_table_string_hash,
> + hash_table_string_compare))
> + {
> + }
> +
> + ~interface_block_definitions()
> + {
> + hash_table_dtor(ht);
> + ralloc_free(mem_ctx);
> + }
> +
> + /**
> + * Lookup the interface definition having the given block name. Return
> + * NULL if none is found.
> + */
> + interface_block_definition *lookup(const char *block_name)
> + {
> + return (interface_block_definition *) hash_table_find(ht, block_name);
> + }
> +
> + /**
> + * Add a new interface definition.
> + */
> + void store(const interface_block_definition &def)
> + {
> + interface_block_definition *hash_entry =
> + rzalloc(mem_ctx, interface_block_definition);
> + *hash_entry = def;
> + hash_table_insert(ht, hash_entry, def.type->name);
> + }
> +
> +private:
> + /**
> + * Ralloc context for data structures allocated by this class.
> + */
> + void *mem_ctx;
> +
> + /**
> + * Hash table mapping interface block name to an \c
> + * interface_block_definition struct. interface_block_definition structs
> + * are allocated using \c mem_ctx.
> + */
> + hash_table *ht;
> +};
> +
> +
> +}; /* anonymous namespace */
> +
>
> void
> validate_intrastage_interface_blocks(struct gl_shader_program *prog,
> const gl_shader **shader_list,
> unsigned num_shaders)
> {
> - glsl_symbol_table interfaces;
> + interface_block_definitions in_interfaces;
> + interface_block_definitions out_interfaces;
> + interface_block_definitions uniform_interfaces;
>
> for (unsigned int i = 0; i < num_shaders; i++) {
> if (shader_list[i] == NULL)
> @@ -52,17 +258,36 @@ validate_intrastage_interface_blocks(struct
> gl_shader_program *prog,
> if (iface_type == NULL)
> continue;
>
> - const glsl_type *old_iface_type =
> - interfaces.get_interface(iface_type->name,
> - (enum ir_variable_mode) var->mode);
> + interface_block_definitions *definitions;
> + switch (var->mode) {
> + case ir_var_shader_in:
> + definitions = &in_interfaces;
> + break;
> + case ir_var_shader_out:
> + definitions = &out_interfaces;
> + break;
> + case ir_var_uniform:
> + definitions = &uniform_interfaces;
> + break;
> + default:
> + /* Only in, out, and uniform interfaces are legal, so we should
> + * never get here.
> + */
> + assert(!"illegal interface type");
> + continue;
> + }
>
> - if (old_iface_type == NULL) {
> + const interface_block_definition def(var);
> + interface_block_definition *prev_def =
> + definitions->lookup(iface_type->name);
> +
> + if (prev_def == NULL) {
> /* This is the first time we've seen the interface, so save
> - * it into our symbol table.
> + * it into the appropriate data structure.
> */
> - interfaces.add_interface(iface_type->name, iface_type,
> - (enum ir_variable_mode) var->mode);
> - } else if (old_iface_type != iface_type) {
> + definitions->store(def);
> + } else if (!intrastage_match(prev_def, &def,
> + (ir_variable_mode) var->mode)) {
> linker_error(prog, "definitions of interface block `%s' do not"
> " match\n", iface_type->name);
> return;
> @@ -76,7 +301,9 @@ validate_interstage_interface_blocks(struct
> gl_shader_program *prog,
> const gl_shader *producer,
> const gl_shader *consumer)
> {
> - glsl_symbol_table interfaces;
> + interface_block_definitions inout_interfaces;
> + interface_block_definitions uniform_interfaces;
> + bool extra_array_level = consumer->Type == GL_GEOMETRY_SHADER;
I'd either make this const or just put the 'consumer->Type ==
GL_GEOMETRY_SHADER' inline at the one place that uses extra_array_level.
>
> /* Add non-output interfaces from the consumer to the symbol table. */
> foreach_list(node, consumer->ir) {
> @@ -84,9 +311,10 @@ validate_interstage_interface_blocks(struct
> gl_shader_program *prog,
> if (!var || !var->get_interface_type() || var->mode ==
> ir_var_shader_out)
> continue;
>
> - interfaces.add_interface(var->get_interface_type()->name,
> - var->get_interface_type(),
> - (enum ir_variable_mode) var->mode);
> + interface_block_definitions *definitions = var->mode == ir_var_uniform
> ?
> + &uniform_interfaces : &inout_interfaces;
> + const interface_block_definition def(var);
> + definitions->store(def);
Is it legal to just do
definitions->store(interface_block_definition(var));
> }
>
> /* Verify that the producer's interfaces match. */
> @@ -95,17 +323,29 @@ validate_interstage_interface_blocks(struct
> gl_shader_program *prog,
> if (!var || !var->get_interface_type() || var->mode ==
> ir_var_shader_in)
> continue;
>
> - enum ir_variable_mode consumer_mode =
> - var->mode == ir_var_uniform ? ir_var_uniform : ir_var_shader_in;
> - const glsl_type *expected_type =
> - interfaces.get_interface(var->get_interface_type()->name,
> - consumer_mode);
> + interface_block_definitions *definitions = var->mode == ir_var_uniform
> ?
> + &uniform_interfaces : &inout_interfaces;
> + interface_block_definition *consumer_def =
> + definitions->lookup(var->get_interface_type()->name);
>
> /* The consumer doesn't use this output block. Ignore it. */
> - if (expected_type == NULL)
> + if (consumer_def == NULL)
> continue;
>
> - if (var->get_interface_type() != expected_type) {
> + const interface_block_definition producer_def(var);
> + bool match;
> + if (var->mode == ir_var_uniform) {
> + /* Uniform matching rules are the same for interstage and intrastage
> + * linking.
> + */
> + match = intrastage_match(consumer_def, &producer_def,
> + (ir_variable_mode) var->mode);
> + } else {
> + match = interstage_match(&producer_def, consumer_def,
> + extra_array_level);
> + }
> +
> + if (!match) {
> linker_error(prog, "definitions of interface block `%s' do not "
> "match\n", var->get_interface_type()->name);
> return;
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
