glsl: merge layouts into the default one as the last step in interface blocks

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.

V2: added a default_layout variable instead of a layout_helper and
    make the merge directly over the layout one. Suggested by Timothy.

Reviewed-by: Timothy Arceri <timothy.arceri@collabora.com>
Signed-off-by: Andres Gomez <agomez@igalia.com>

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 55f9a6c2e5aeb6168047e14ed542e81d4d921d59..e7c3aff6b82fdd8740fce9e2265008696f945163 100644 (file)
--- 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,
@@ -1139,6 +1141,7 @@ public:
    virtual ir_rvalue *hir(exec_list *instructions,
                          struct _mesa_glsl_parse_state *state);
 
+   ast_type_qualifier default_layout;
    ast_type_qualifier layout;
    const char *block_name;
 

diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
index 478e4a8147bccec679c158542b8ae872595d341f..48afe09929b20f6649f254e61f4eb661c989130a 100644 (file)
--- 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 a48dc68b00ea2a7943e2e97adfeb23ae19c0265f..f0f212c1722de2346b4ed598f2fc44673bbc72e1 100644 (file)
--- a/src/compiler/glsl/glsl_parser.yy
+++ b/src/compiler/glsl/glsl_parser.yy
@@ -838,6 +838,14 @@ declaration:
    }
    | interface_block
    {
+      ast_interface_block *block = (ast_interface_block *) $1;
+      if (block->layout.has_layout() || block->layout.has_memory()) {
+         if (!block->default_layout.merge_qualifier(& @1, state, block->layout, false)) {
+            YYERROR;
+         }
+      }
+      block->layout = block->default_layout;
+
       $$ = $1;
    }
    ;
@@ -2705,17 +2713,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.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, false)) {
          YYERROR;
       }
 
-      block->layout.is_default_qualifier = false;
+      block->layout = $1;
 
       $$ = block;
    }
@@ -2723,14 +2730,15 @@ interface_block:
    {
       ast_interface_block *block = (ast_interface_block *)$2;
 
-      if (!block->layout.flags.q.buffer) {
+      if (!block->default_layout.flags.q.buffer) {
             _mesa_glsl_error(& @1, state,
                              "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, false)) {
          YYERROR;
       }
+      block->layout = $1;
       $$ = block;
    }
    ;
@@ -2741,9 +2749,9 @@ basic_interface_block:
       ast_interface_block *const block = $6;
 
       if ($1.flags.q.uniform) {
-         block->layout = *state->default_uniform_qualifier;
+         block->default_layout = *state->default_uniform_qualifier;
       } else if ($1.flags.q.buffer) {
-         block->layout = *state->default_shader_storage_qualifier;
+         block->default_layout = *state->default_shader_storage_qualifier;
       }
       block->block_name = $2;
       block->declarations.push_degenerate_list_at_head(& $4->link);

diff --git a/src/compiler/glsl/glsl_parser_extras.cpp b/src/compiler/glsl/glsl_parser_extras.cpp
index 85a2e943ff51af27bcb6feab61047293e00de6ce..78f7fe259654395232e43477ad504c1b7d67cae2 100644 (file)
--- a/src/compiler/glsl/glsl_parser_extras.cpp
+++ b/src/compiler/glsl/glsl_parser_extras.cpp
@@ -276,12 +276,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;
@@ -1003,22 +1001,22 @@ _mesa_ast_process_interface_block(YYLTYPE *locp,
     */
    uint64_t block_interface_qualifier = q.flags.i;
 
-   block->layout.flags.i |= block_interface_qualifier;
+   block->default_layout.flags.i |= block_interface_qualifier;
 
    if (state->stage == MESA_SHADER_GEOMETRY &&
        state->has_explicit_attrib_stream() &&
-       block->layout.flags.q.out) {
+       block->default_layout.flags.q.out) {
       /* Assign global layout's stream value. */
-      block->layout.flags.q.stream = 1;
-      block->layout.flags.q.explicit_stream = 0;
-      block->layout.stream = state->out_qualifier->stream;
+      block->default_layout.flags.q.stream = 1;
+      block->default_layout.flags.q.explicit_stream = 0;
+      block->default_layout.stream = state->out_qualifier->stream;
    }
 
-   if (state->has_enhanced_layouts() && block->layout.flags.q.out) {
+   if (state->has_enhanced_layouts() && block->default_layout.flags.q.out) {
       /* Assign global layout's xfb_buffer value. */
-      block->layout.flags.q.xfb_buffer = 1;
-      block->layout.flags.q.explicit_xfb_buffer = 0;
-      block->layout.xfb_buffer = state->out_qualifier->xfb_buffer;
+      block->default_layout.flags.q.xfb_buffer = 1;
+      block->default_layout.flags.q.explicit_xfb_buffer = 0;
+      block->default_layout.xfb_buffer = state->out_qualifier->xfb_buffer;
    }
 
    foreach_list_typed (ast_declarator_list, member, link, &block->declarations) {