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

[Kenneth Graunke]

On 05/16/2013 05:44 PM, Jordan Justen wrote:
> Verify that interface blocks match when combining compilation
> units at the same stage. (For example, when merging all vertex
> shaders.)
>
> Fixes piglit glsl-1.50 test:
> * linker/interface-blocks-multiple-vs-member-count-mismatch.shader_test
>
> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>

I will be sending out a revised version of this patch that incorporates 
my suggestions.

> ---
>   src/glsl/Makefile.sources     |    1 +
>   src/glsl/interface_blocks.cpp |   86 +++++++++++++++++++++++++++++++++++++++++
>   src/glsl/interface_blocks.h   |   32 +++++++++++++++
>   src/glsl/linker.cpp           |    7 ++++
>   4 files changed, 126 insertions(+)
>   create mode 100644 src/glsl/interface_blocks.cpp
>   create mode 100644 src/glsl/interface_blocks.h
>
> diff --git a/src/glsl/Makefile.sources b/src/glsl/Makefile.sources
> index b9fd283..e39cb1a 100644
> --- a/src/glsl/Makefile.sources
> +++ b/src/glsl/Makefile.sources
> @@ -26,6 +26,7 @@ LIBGLSL_FILES = \
>   	$(GLSL_SRCDIR)/glsl_types.cpp \
>   	$(GLSL_SRCDIR)/glsl_symbol_table.cpp \
>   	$(GLSL_SRCDIR)/hir_field_selection.cpp \
> +	$(GLSL_SRCDIR)/interface_blocks.cpp \
>   	$(GLSL_SRCDIR)/ir_basic_block.cpp \
>   	$(GLSL_SRCDIR)/ir_builder.cpp \
>   	$(GLSL_SRCDIR)/ir_clone.cpp \
> diff --git a/src/glsl/interface_blocks.cpp b/src/glsl/interface_blocks.cpp
> new file mode 100644
> index 0000000..6dfd0c4
> --- /dev/null
> +++ b/src/glsl/interface_blocks.cpp

I would call this link_interface_blocks.cpp.  All the other linker code 
is named link*.cpp.

> @@ -0,0 +1,86 @@
> +/*
> + * Copyright (c) 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +/**
> + * \file interface_blocks.cpp
> + * GLSL interface blocks support
> + */
> +
> +#include "ir.h"
> +#include "glsl_symbol_table.h"
> +#include "main/macros.h"
> +#include "program/hash_table.h"
> +
> +static bool
> +cross_validate_interface_blocks(const gl_shader **shader_list,
> +                                unsigned num_shaders)
> +{
> +   bool ok = true;
> +   glsl_symbol_table interfaces;
> +
> +   for (unsigned int i = 0; ok && i < num_shaders; i++) {
> +      if (shader_list[i] == NULL)
> +         continue;
> +
> +      foreach_list(node, shader_list[i]->ir) {
> +         ir_variable *var = ((ir_instruction *) node)->as_variable();
> +         if (!var)
> +            continue;
> +
> +         const glsl_type *iface_type = var->interface_type;
> +
> +         if (iface_type == NULL)
> +            continue;
> +
> +         const char *iface_name = iface_type->name;
> +
> +         const glsl_type *old_iface_type =
> +            interfaces.get_interface(iface_name,
> +                                     (enum ir_variable_mode) var->mode);
> +
> +         /* This is the first time we've seen the interface, so save
> +          * it into our symbol table.
> +          */
> +         if (old_iface_type == NULL) {
> +            interfaces.add_interface(iface_name, iface_type,
> +                                     (enum ir_variable_mode) var->mode);
> +            old_iface_type = iface_type;
> +         }
> +
> +         ok &= old_iface_type == iface_type;

I guess C++ booleans are guaranteed to be 0 or 1, so this ought to work, 
but using bitwise operators on bools is still a bit odd.

> +         if (!ok)
> +            break;

You could actually just drop the "ok" variable altogether and change 
this code to:

if (old_iface_type != iface_type)
    return false;

and then "return true" at the bottom.  Seems simpler.

> +      }
> +   }
> +
> +   return ok;
> +}
> +
> +bool
> +validate_intrastage_interface_blocks(const gl_shader **shader_list,
> +                                     unsigned num_shaders)
> +{
> +   return cross_validate_interface_blocks(shader_list,
> +                                          num_shaders);
> +}
> +
> diff --git a/src/glsl/interface_blocks.h b/src/glsl/interface_blocks.h
> new file mode 100644
> index 0000000..4c37c02
> --- /dev/null
> +++ b/src/glsl/interface_blocks.h

I would just stuff this in linker.h.  Two prototypes is probably not 
worth the extra header file.

> @@ -0,0 +1,32 @@
> +/*
> + * Copyright (c) 2013 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#pragma once
> +#ifndef GLSL_INTERFACE_BLOCKS_H
> +#define GLSL_INTERFACE_BLOCKS_H
> +
> +bool
> +validate_intrastage_interface_blocks(const gl_shader **shader_list,
> +                                     unsigned num_shaders);
> +
> +#endif /* GLSL_INTERFACE_BLOCKS_H */
> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> index c8d430c..4f2cab2 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -66,6 +66,7 @@
>
>   #include "main/core.h"
>   #include "glsl_symbol_table.h"
> +#include "interface_blocks.h"
>   #include "ir.h"
>   #include "program.h"
>   #include "program/hash_table.h"
> @@ -938,6 +939,12 @@ link_intrastage_shaders(void *mem_ctx,
>      if (!cross_validate_globals(prog, shader_list, num_shaders, false))
>         return NULL;
>
> +   /* Check that interface blocks defined in multiple shaders are consistent.
> +    */
> +   if (!validate_intrastage_interface_blocks((const gl_shader **)shader_list,
> +                                             num_shaders))
> +      return NULL;
> +
>      /* Check that uniform blocks between shaders for a stage agree. */
>      const int num_uniform_blocks =
>         link_uniform_blocks(mem_ctx, prog, shader_list, num_shaders,
>