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 <ago...@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;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
