Re: [Mesa-dev] [PATCH 2/5] glsl: last duplicated layout-qualifier-name in multiple layout-qualifiers overrides the former

[Andres Gomez]

On Tue, 2016-10-11 at 14:13 +1100, Timothy Arceri wrote:
> Again the subject should say what the change is rather than making an
> assertion.
> 
> How about:
> 
> glsl: ignore all but the rightmost layout qualifier name from the
> rightmost layout qualifier

Thanks! Applied in the v2 series to come ...

> On Fri, 2016-10-07 at 01:52 +0300, Andres Gomez wrote:
> > From page 46 (page 52 of the PDF) of the GLSL 4.20 spec:
> > 
> >   " More than one layout qualifier may appear in a single
> > declaration. If the same layout-qualifier-name occurs in multiple
> > layout qualifiers for the same declaration, the last one
> > overrides
> > the former ones."
> > 
> > Consider this example:
> > 
> >   " #version 150
> > #extension GL_ARB_shading_language_420pack: enable
> > #extension GL_ARB_enhanced_layouts: enable
> > 
> > layout(max_vertices=2) layout(max_vertices=3) out;
> > layout(max_vertices=3) out;"
> > 
> > Although different values for the "max_vertices" layout-qualifier-
> > name
> > should end in a compilation failure, since only the last occurrence
> > is
> > taken into account, this small piece of code from a shader is valid.
> > 
> > Hence, when merging qualifiers in an ast_type_qualifier, we now
> > ignore
> > new appearances of a same layout-qualifier-name if the new
> > "is_multiple_layouts_merge" parameter is on, since the GLSL parser
> > works in this case from right to left.
> 
> 
> I think you should change the new param to be
> "merging_single_declaration" this will make the code much easier to
> follow.
> 
> merging_single_declaration would be true for
> either is_multiple_layouts_merge or is_single_layout_merge in your
> current code. You will also need to make the changes I suggest below
> for the 420_pack detection for this to work.

No, that's not posible.

Duplicate layout-qualifier-names can happen because of them happening
in the same layout-qualifier (supported by arb_enhanced_layouts only,
is_single_layout_merge flag) or because of them happening across
multiple layout-qualifiers in the same declaration (supported
by arb_enhanced_layouts and arb_shading_language_420pack,
is_multiple_layouts_merge flag).

Therefore, we will spit an error depending on the supported extension
and how the duplicate happens. That's why we need both flags.

> > Also, the GLSL parser has been simplified to check for the needed
> > GL_ARB_shading_language_420pack extension just when merging the
> > qualifiers in the proper cases.
> 
> 
> Can you please split this into a separate patch that comes before this
> one, that will help make this patch easier to follow. I have some
> suggestions for improving this below.

Done.

[split]

> > diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> > index 4c648d0..73c73b7 100644
> > --- a/src/compiler/glsl/ast.h
> > +++ b/src/compiler/glsl/ast.h
> > @@ -382,7 +382,7 @@ public:
> >  
> > void merge_qualifier(ast_layout_expression *l_expr)
> > {
> > -  layout_const_expressions.append_list(_expr-
> > > layout_const_expressions);
> > 
> > +  layout_const_expressions.prepend_list(_expr-
> > > layout_const_expressions);
> 
> 
> Does this really do anything? process_qualifier_constant() should just
> process the whole list the order shouldn't matter.

That's a very good observation.

There were 2 patches from Dave that were suggesting that the
ast_layout_expression could hold different values and that would be OK
depending on the layout-qualifier-name. See the patch 4/5 of the
series.

If that would be the case, it would make sense to return just the last
declared value of the layout-qualifier-name in the
process_qualifier_constant. The way the iteration algorithm there was
implemented was returning the first occurrence, not the last. This was
fixing that.

However, after your comment and more thinking I've realized that it was
not making sense. Hence, this is not needed, as you say, and I've
decided to drop my 4/5 patch, but also the 2 patches from Dave. That
will come in the v2 series.

[snip]

> > diff --git a/src/compiler/glsl/ast_type.cpp
> > b/src/compiler/glsl/ast_type.cpp
> > index 504b533..f02f71b 100644
> > --- a/src/compiler/glsl/ast_type.cpp
> > +++ b/src/compiler/glsl/ast_type.cpp

[snip]

> > @@ -186,6 +192,12 @@ ast_type_qualifier::merge_qualifier(YYLTYPE
> > *loc,
> >    return false;
> > }
> >  
> > +   if (is_multiple_layouts_merge && !state->has_420pack_or_es31()) {
> > +  _mesa_glsl_error(loc, state,
> > +   "duplicate layout(...) qualifiers");
> > +  return false;
> > +   }
> > 
> 
> 
> You should just be able to do:
> 
>    if (!is_single_layout_merge && has_layout() &&
>        !is_default_qualifier && !state->has_420pack_or_es31())
>       _mesa_glsl_error(loc, state,
>                        "duplicate layout(...) qualifiers");
>       return false;
>    }
> 
> That way we cover everything and don't need the
> is_muliple_layouts_merge param.

As commented above, 

Re: [Mesa-dev] [PATCH 2/5] glsl: last duplicated layout-qualifier-name in multiple layout-qualifiers overrides the former

[Timothy Arceri]

Again the subject should say what the change is rather than making an
assertion.

How about:

glsl: ignore all but the rightmost layout qualifier name from the
rightmost layout qualifier

On Fri, 2016-10-07 at 01:52 +0300, Andres Gomez wrote:
> From page 46 (page 52 of the PDF) of the GLSL 4.20 spec:
> 
>   " More than one layout qualifier may appear in a single
> declaration. If the same layout-qualifier-name occurs in multiple
> layout qualifiers for the same declaration, the last one
> overrides
> the former ones."
> 
> Consider this example:
> 
>   " #version 150
> #extension GL_ARB_shading_language_420pack: enable
> #extension GL_ARB_enhanced_layouts: enable
> 
> layout(max_vertices=2) layout(max_vertices=3) out;
> layout(max_vertices=3) out;"
> 
> Although different values for the "max_vertices" layout-qualifier-
> name
> should end in a compilation failure, since only the last occurrence
> is
> taken into account, this small piece of code from a shader is valid.
> 
> Hence, when merging qualifiers in an ast_type_qualifier, we now
> ignore
> new appearances of a same layout-qualifier-name if the new
> "is_multiple_layouts_merge" parameter is on, since the GLSL parser
> works in this case from right to left.

I think you should change the new param to be
"merging_single_declaration" this will make the code much easier to
follow.

merging_single_declaration would be true for
either is_multiple_layouts_merge or is_single_layout_merge in your
current code. You will also need to make the changes I suggest below
for the 420_pack detection for this to work.

> 
> Also, the GLSL parser has been simplified to check for the needed
> GL_ARB_shading_language_420pack extension just when merging the
> qualifiers in the proper cases.

Can you please split this into a separate patch that comes before this
one, that will help make this patch easier to follow. I have some
suggestions for improving this below.

> 
> In addition, any special treatment for the buffer, uniform, in or out
> layout defaults has been moved in the GLSL parser to the rule
> triggered just after any previous processing/merging on the
> layout-qualifiers has happened in a single declaration since it was
> run too soon previously.
> 
> Finally, the merging of an ast_layout_expression is now done
> prepending instead of appending since the processing of the qualifier
> constant returns the first value in the list and the last appearing
> declaration of a variable or default overrides the previous
> declarations.
> 
> Fixes GL44-CTS.shading_language_420pack.qualifier_override_layout
> 
> Signed-off-by: Andres Gomez 
> ---
>  src/compiler/glsl/ast.h  |   5 +-
>  src/compiler/glsl/ast_type.cpp   |  34 +++---
>  src/compiler/glsl/glsl_parser.yy | 131 +++
> 
>  3 files changed, 77 insertions(+), 93 deletions(-)
> 
> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> index 4c648d0..73c73b7 100644
> --- a/src/compiler/glsl/ast.h
> +++ b/src/compiler/glsl/ast.h
> @@ -382,7 +382,7 @@ public:
>  
> void merge_qualifier(ast_layout_expression *l_expr)
> {
> -  layout_const_expressions.append_list(_expr-
> >layout_const_expressions);
> +  layout_const_expressions.prepend_list(_expr-
> >layout_const_expressions);

Does this really do anything? process_qualifier_constant() should just
process the whole list the order shouldn't matter.

> }
>  
> exec_list layout_const_expressions;
> @@ -746,7 +746,8 @@ struct ast_type_qualifier {
> bool merge_qualifier(YYLTYPE *loc,
>   _mesa_glsl_parse_state *state,
>  const ast_type_qualifier ,
> -bool is_single_layout_merge);
> +bool is_single_layout_merge,
> +bool is_multiple_layouts_merge = false);
>  
> bool merge_out_qualifier(YYLTYPE *loc,
> _mesa_glsl_parse_state *state,
> diff --git a/src/compiler/glsl/ast_type.cpp
> b/src/compiler/glsl/ast_type.cpp
> index 504b533..f02f71b 100644
> --- a/src/compiler/glsl/ast_type.cpp
> +++ b/src/compiler/glsl/ast_type.cpp
> @@ -108,15 +108,21 @@ ast_type_qualifier::has_auxiliary_storage()
> const
>  }
>  
>  /**
> - * This function merges both duplicate identifies within a single
> layout and
> - * multiple layout qualifiers on a single variable declaration. The
> - * is_single_layout_merge param is used differentiate between the
> two.
> + * This function merges duplicate layout identifiers.
> + *
> + * It deals with duplicates within a single layout qualifier, among
> multiple
> + * layout qualifiers on a single declaration and on several
> declarations for
> + * the same variable.
> + *
> + * The is_single_layout_merge and is_multiple_layouts_merge
> parameters are
> + * used to differentiate among them.
>   */
>  bool
>  ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> 

[Mesa-dev] [PATCH 2/5] glsl: last duplicated layout-qualifier-name in multiple layout-qualifiers overrides the former

[Andres Gomez]

From page 46 (page 52 of the PDF) of the GLSL 4.20 spec:

  " More than one layout qualifier may appear in a single
declaration. If the same layout-qualifier-name occurs in multiple
layout qualifiers for the same declaration, the last one overrides
the former ones."

Consider this example:

  " #version 150
#extension GL_ARB_shading_language_420pack: enable
#extension GL_ARB_enhanced_layouts: enable

layout(max_vertices=2) layout(max_vertices=3) out;
layout(max_vertices=3) out;"

Although different values for the "max_vertices" layout-qualifier-name
should end in a compilation failure, since only the last occurrence is
taken into account, this small piece of code from a shader is valid.

Hence, when merging qualifiers in an ast_type_qualifier, we now ignore
new appearances of a same layout-qualifier-name if the new
"is_multiple_layouts_merge" parameter is on, since the GLSL parser
works in this case from right to left.

Also, the GLSL parser has been simplified to check for the needed
GL_ARB_shading_language_420pack extension just when merging the
qualifiers in the proper cases.

In addition, any special treatment for the buffer, uniform, in or out
layout defaults has been moved in the GLSL parser to the rule
triggered just after any previous processing/merging on the
layout-qualifiers has happened in a single declaration since it was
run too soon previously.

Finally, the merging of an ast_layout_expression is now done
prepending instead of appending since the processing of the qualifier
constant returns the first value in the list and the last appearing
declaration of a variable or default overrides the previous
declarations.

Fixes GL44-CTS.shading_language_420pack.qualifier_override_layout

Signed-off-by: Andres Gomez 
---
 src/compiler/glsl/ast.h  |   5 +-
 src/compiler/glsl/ast_type.cpp   |  34 +++---
 src/compiler/glsl/glsl_parser.yy | 131 +++
 3 files changed, 77 insertions(+), 93 deletions(-)

diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
index 4c648d0..73c73b7 100644
--- a/src/compiler/glsl/ast.h
+++ b/src/compiler/glsl/ast.h
@@ -382,7 +382,7 @@ public:
 
void merge_qualifier(ast_layout_expression *l_expr)
{
-  layout_const_expressions.append_list(_expr->layout_const_expressions);
+  layout_const_expressions.prepend_list(_expr->layout_const_expressions);
}
 
exec_list layout_const_expressions;
@@ -746,7 +746,8 @@ struct ast_type_qualifier {
bool merge_qualifier(YYLTYPE *loc,
_mesa_glsl_parse_state *state,
 const ast_type_qualifier ,
-bool is_single_layout_merge);
+bool is_single_layout_merge,
+bool is_multiple_layouts_merge = false);
 
bool merge_out_qualifier(YYLTYPE *loc,
_mesa_glsl_parse_state *state,
diff --git a/src/compiler/glsl/ast_type.cpp b/src/compiler/glsl/ast_type.cpp
index 504b533..f02f71b 100644
--- a/src/compiler/glsl/ast_type.cpp
+++ b/src/compiler/glsl/ast_type.cpp
@@ -108,15 +108,21 @@ ast_type_qualifier::has_auxiliary_storage() const
 }
 
 /**
- * This function merges both duplicate identifies within a single layout and
- * multiple layout qualifiers on a single variable declaration. The
- * is_single_layout_merge param is used differentiate between the two.
+ * This function merges duplicate layout identifiers.
+ *
+ * It deals with duplicates within a single layout qualifier, among multiple
+ * layout qualifiers on a single declaration and on several declarations for
+ * the same variable.
+ *
+ * The is_single_layout_merge and is_multiple_layouts_merge parameters are
+ * used to differentiate among them.
  */
 bool
 ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
 _mesa_glsl_parse_state *state,
 const ast_type_qualifier ,
-bool is_single_layout_merge)
+bool is_single_layout_merge,
+bool is_multiple_layouts_merge)
 {
ast_type_qualifier ubo_mat_mask;
ubo_mat_mask.flags.i = 0;
@@ -186,6 +192,12 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
   return false;
}
 
+   if (is_multiple_layouts_merge && !state->has_420pack_or_es31()) {
+  _mesa_glsl_error(loc, state,
+   "duplicate layout(...) qualifiers");
+  return false;
+   }
+
if (q.flags.q.prim_type) {
   if (this->flags.q.prim_type && this->prim_type != q.prim_type) {
  _mesa_glsl_error(loc, state,
@@ -196,7 +208,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
}
 
if (q.flags.q.max_vertices) {
-  if (this->max_vertices && !is_single_layout_merge) {
+  if (this->max_vertices
+  && !is_single_layout_merge && !is_multiple_layouts_merge) {