[Mesa-dev] [PATCH v4 17/17] glsl linker: compare interface blocks during interstage linking

[Kenneth Graunke]

On 05/16/2013 05:44 PM, Jordan Justen wrote:
> Verify that interface blocks match when linking separate shader
> stages into a program.
>
> Fixes piglit glsl-1.50 tests:
> * linker/interface-blocks-vs-fs-member-count-mismatch.shader_test
> * linker/interface-blocks-vs-fs-member-order-mismatch.shader_test
>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> ---
>   src/glsl/interface_blocks.cpp |   47 +++++++++++++++++++++++++++++++++++++++--
>   src/glsl/interface_blocks.h   |    4 ++++
>   src/glsl/linker.cpp           |    6 ++++++
>   3 files changed, 55 insertions(+), 2 deletions(-)

In my mind, validating interface blocks within a single stage and 
between two adjacent stages are really separate tasks that work 
differently.  Intrastage has an arbitrarily long list of, say, vertex 
shaders; interstage has exactly two.

I really dislike how this patch conflates the two.  I also find creating 
a list with exactly two elements, looping over it, and special-casing 
every element to be quite ugly.

Sorry for taking so long on this series and coming in with harsh 
feedback. :(

I'll send out a proposed v5 that does this a bit differently and still 
passes all of your piglit tests.

> diff --git a/src/glsl/interface_blocks.cpp b/src/glsl/interface_blocks.cpp
> index 6dfd0c4..b458b59 100644
> --- a/src/glsl/interface_blocks.cpp
> +++ b/src/glsl/interface_blocks.cpp
> @@ -30,14 +30,21 @@
>   #include "glsl_symbol_table.h"
>   #include "main/macros.h"
>   #include "program/hash_table.h"
> +#include "linker.h"
>
>   static bool
>   cross_validate_interface_blocks(const gl_shader **shader_list,
> -                                unsigned num_shaders)
> +                                unsigned num_shaders,
> +                                bool interstage)
>   {
>      bool ok = true;
>      glsl_symbol_table interfaces;
>
> +   /* Interstage linking checks assume 2 shaders. First the producer, and
> +    * then the consumer.
> +    */
> +   assert(!interstage || num_shaders == 2);
> +
>      for (unsigned int i = 0; ok && i < num_shaders; i++) {
>         if (shader_list[i] == NULL)
>            continue;
> @@ -52,6 +59,18 @@ cross_validate_interface_blocks(const gl_shader **shader_list,
>            if (iface_type == NULL)
>               continue;
>
> +         /* If we are checking interstage linking then we assume:
> +          *  * The first shader (producer) has i == 0, and for the
> +          *    producer we don't need to check input interfaces.
> +          *  * The second shader (consumer) has i == 1, and for the
> +          *    consumer we don't need to check output interfaces.
> +          */
> +         if (interstage &&
> +             ((var->mode == ir_var_shader_in && i == 0) ||
> +              (var->mode == ir_var_shader_out && i == 1))
> +            )
> +            continue;
> +
>            const char *iface_name = iface_type->name;
>
>            const glsl_type *old_iface_type =
> @@ -70,6 +89,18 @@ cross_validate_interface_blocks(const gl_shader **shader_list,
>            ok &= old_iface_type == iface_type;
>            if (!ok)
>               break;
> +
> +         /* For interstage linking, if the interface is an input, then
> +          * we need to verify that its type matches a previously declared
> +          * output type.
> +          */
> +         if (interstage && var->mode == ir_var_shader_in) {
> +            old_iface_type =
> +               interfaces.get_interface(iface_name, ir_var_shader_out);
> +            ok &= old_iface_type == iface_type;
> +            if (!ok)
> +               break;
> +         }
>         }
>      }
>
> @@ -81,6 +112,18 @@ validate_intrastage_interface_blocks(const gl_shader **shader_list,
>                                        unsigned num_shaders)
>   {
>      return cross_validate_interface_blocks(shader_list,
> -                                          num_shaders);
> +                                          num_shaders,
> +                                          false);
> +}
> +
> +bool
> +validate_interstage_interface_blocks(const gl_shader *producer,
> +                                     const gl_shader *consumer)
> +{
> +   const gl_shader *shader_list[] = { producer, consumer };
> +
> +   return cross_validate_interface_blocks((const gl_shader **) shader_list,
> +                                          ARRAY_SIZE(shader_list),
> +                                          true);
>   }
>
> diff --git a/src/glsl/interface_blocks.h b/src/glsl/interface_blocks.h
> index 4c37c02..58c847c 100644
> --- a/src/glsl/interface_blocks.h
> +++ b/src/glsl/interface_blocks.h
> @@ -29,4 +29,8 @@ bool
>   validate_intrastage_interface_blocks(const gl_shader **shader_list,
>                                        unsigned num_shaders);
>
> +bool
> +validate_interstage_interface_blocks(const gl_shader *producer,
> +                                     const gl_shader *consumer);
> +
>   #endif /* GLSL_INTERFACE_BLOCKS_H */
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index 4f2cab2..71203b1 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -1729,6 +1729,12 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>   	 if (prog->_LinkedShaders[i] == NULL)
>   	    continue;
>
> +         if (!validate_interstage_interface_blocks(prog->_LinkedShaders[prev],
> +                                                   prog->_LinkedShaders[i])) {
> +            linker_error(prog, "interface block mismatch between shader stages\n");
> +            goto done;
> +         }
> +
>   	 if (!cross_validate_outputs_to_inputs(prog,
>   					       prog->_LinkedShaders[prev],
>   					       prog->_LinkedShaders[i]))
>