[Mesa-dev] [PATCH 2/9] glsl: merge layouts into the default one as the last step in interface blocks

[Timothy Arceri]

On Sat, 2016-10-22 at 23:09 +0300, Andres Gomez wrote:
> Consider this example:
> 
>     " #version 150 core
>       #extension GL_ARB_shading_language_420pack: require
>       #extension GL_ARB_explicit_attrib_location: require
> 
>       layout(location=0) out vec4 o;
>       layout(binding=2) layout(binding=3, std140) uniform U {
>           vec4 a;
>       } u[2];"
> 
> As there is 2 layout-qualifiers for the uniform U and the binding
> layout-qualifier-id is duplicated, the rules set by the
> ARB_shading_language_420pack spec state that the rightmost should
> prevail.
> 
> Our ast_type_qualifier merges with others in a way that if the value
> for a layout-qualifier-id is set in both, the object being merged
> overwrites the value of the object invoking the merge. Hence, the
> merge has to happen from the left layout towards the right one and
> this was not happening for interface blocks because we were merging
> into the default layout qualifier.
> 
> Now, the merge is done from left to right and, as a last step, we
> merge into the default layout qualifier if needed, so the values of
> the explicit layouts prevail over it.
> 
> Signed-off-by: Andres Gomez <agomez at igalia.com>

That patch looks logically correct. However I'd really like it if you
could rename the existing layout member to default_layout and make the
new one layout rather than layout_helper.

I understand this will likely be a few more changes but the end result
will be much easier to follow.


> ---
>  src/compiler/glsl/ast.h                  |  9 ++++++---
>  src/compiler/glsl/ast_type.cpp           |  9 +++++++++
>  src/compiler/glsl/glsl_parser.yy         | 16 +++++++++++-----
>  src/compiler/glsl/glsl_parser_extras.cpp |  2 --
>  4 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index 55f009a..54119b7 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -725,9 +725,6 @@ struct ast_type_qualifier {
>      */
>     glsl_base_type image_base_type;
>  
> -   /** Flag to know if this represents a default value for a
> qualifier */
> -   bool is_default_qualifier;
> -
>     /**
>      * Return true if and only if an interpolation qualifier is
> present.
>      */
> @@ -748,6 +745,11 @@ struct ast_type_qualifier {
>      */
>     bool has_auxiliary_storage() const;
>  
> +   /**
> +    * Return true if and only if a memory qualifier is present.
> +    */
> +   bool has_memory() const;
> +
>     bool merge_qualifier(YYLTYPE *loc,
>  			_mesa_glsl_parse_state *state,
>                          const ast_type_qualifier &q,
> @@ -1140,6 +1142,7 @@ public:
>  			  struct _mesa_glsl_parse_state *state);
>  
>     ast_type_qualifier layout;
> +   ast_type_qualifier layout_helper;
>     const char *block_name;
>  
>     /**
> diff --git a/src/compiler/glsl/ast_type.cpp
> b/src/compiler/glsl/ast_type.cpp
> index 35acce3..f5bd936 100644
> --- a/src/compiler/glsl/ast_type.cpp
> +++ b/src/compiler/glsl/ast_type.cpp
> @@ -107,6 +107,15 @@ ast_type_qualifier::has_auxiliary_storage()
> const
>            || this->flags.q.patch;
>  }
>  
> +bool ast_type_qualifier::has_memory() const
> +{
> +   return this->flags.q.coherent
> +          || this->flags.q._volatile
> +          || this->flags.q.restrict_flag
> +          || this->flags.q.read_only
> +          || this->flags.q.write_only;
> +}
> +
>  /**
>   * This function merges both duplicate identifies within a single
> layout and
>   * multiple layout qualifiers on a single variable declaration. The
> diff --git a/src/compiler/glsl/glsl_parser.yy
> b/src/compiler/glsl/glsl_parser.yy
> index 38cbd3f..b2e9e6a 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -838,6 +838,12 @@ declaration:
>     }
>     | interface_block
>     {
> +      ast_interface_block *block = (ast_interface_block *) $1;
> +      if (block->layout_helper.has_layout() || block-
> >layout_helper.has_memory()) {
> +         if (!block->layout.merge_qualifier(& @1, state, block-
> >layout_helper, false)) {
> +            YYERROR;
> +         }
> +      }
>        $$ = $1;
>     }
>     ;
> @@ -2701,17 +2707,16 @@ interface_block:
>     {
>        ast_interface_block *block = (ast_interface_block *) $2;
>  
> -      if (!state->has_420pack_or_es31() && block-
> >layout.has_layout() &&
> -          !block->layout.is_default_qualifier) {
> +      if (!state->has_420pack_or_es31() && block-
> >layout_helper.has_layout()) {
>           _mesa_glsl_error(&@1, state, "duplicate layout(...)
> qualifiers");
>           YYERROR;
>        }
>  
> -      if (!block->layout.merge_qualifier(& @1, state, $1, false)) {
> +      if (!$1.merge_qualifier(& @1, state, block->layout_helper,
> false)) {
>           YYERROR;
>        }
>  
> -      block->layout.is_default_qualifier = false;
> +      block->layout_helper = $1;
>  
>        $$ = block;
>     }
> @@ -2724,9 +2729,10 @@ interface_block:
>                               "memory qualifiers can only be used in
> the "
>                               "declaration of shader storage
> blocks");
>        }
> -      if (!block->layout.merge_qualifier(& @1, state, $1, false)) {
> +      if (!$1.merge_qualifier(& @1, state, block->layout_helper,
> false)) {
>           YYERROR;
>        }
> +      block->layout_helper = $1;
>        $$ = block;
>     }
>     ;
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
> b/src/compiler/glsl/glsl_parser_extras.cpp
> index b351180..3b487b3 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -273,12 +273,10 @@
> _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context
> *_ctx,
>     this->default_uniform_qualifier = new(this) ast_type_qualifier();
>     this->default_uniform_qualifier->flags.q.shared = 1;
>     this->default_uniform_qualifier->flags.q.column_major = 1;
> -   this->default_uniform_qualifier->is_default_qualifier = true;
>  
>     this->default_shader_storage_qualifier = new(this)
> ast_type_qualifier();
>     this->default_shader_storage_qualifier->flags.q.shared = 1;
>     this->default_shader_storage_qualifier->flags.q.column_major = 1;
> -   this->default_shader_storage_qualifier->is_default_qualifier =
> true;
>  
>     this->fs_uses_gl_fragcoord = false;
>     this->fs_redeclares_gl_fragcoord = false;