[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(&l_expr-
> > > layout_const_expressions);
> > 
> > +      layout_const_expressions.prepend_list(&l_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, that's not possible.

Just the lines above this addition was the other check for
the is_single_layout_merge flag with the error exit.

[snip]

> > diff --git a/src/compiler/glsl/glsl_parser.yy
> > b/src/compiler/glsl/glsl_parser.yy
> > index 9e1fd9e..f4bf8fc 100644
> > --- a/src/compiler/glsl/glsl_parser.yy
> > +++ b/src/compiler/glsl/glsl_parser.yy

[snip]

> > @@ -2828,43 +2821,59 @@ member_declaration:
> >  layout_uniform_defaults:
> >     layout_qualifier layout_uniform_defaults
> >     {
> > -      $$ = NULL;
> > -      if (!state->has_420pack_or_es31()) {
> > -         _mesa_glsl_error(&@1, state, "duplicate layout(...)
> > qualifiers");
> > +      $$ = $1;
> > +      if (!$$.merge_qualifier(& @2, state, $2, false, true)) {
> >           YYERROR;
> > -      } else {
> > -         if (!state->default_uniform_qualifier->
> > -                merge_qualifier(& @1, state, $1, false)) {
> > -            YYERROR;
> > -         }
> >        }
> >     }
> >     | layout_qualifier UNIFORM ';'
> 
> 
> These all do the same thing now right? So can't we merge them somehow?

Not in the parser in a simple way.

I could create just a call in glsl_parser_extras but I will have to
repeat the call anyway and the gain would be really little.

Thanks for the review!

-- 
Br,

Andres