[Mesa-dev] [PATCH 1/5] glsl: last duplicated layout-qualifier-name in a layout-qualifier overrides the former

[Andres Gomez]

Thanks for the review, I've applied your comments to the coming v2
series.

Br!

On Tue, 2016-10-11 at 13:17 +1100, Timothy Arceri wrote:
> The subject line should describe the change not make an assertion.
> 
> How about: 
> 
> glsl: ignore all but the rightmost layout-qualifier-name
> 
> On Fri, 2016-10-07 at 01:52 +0300, Andres Gomez wrote:
> > In a declaration, when a layout qualifier appears and holds
> > duplicated
> > layout-qualifier-name, only the last occurrence should be taken into
> > account.
> 
> 
> Slight change to above:
> 
> When a layout contains a duplicated layout-qualifier-name in a single
> declaration, only the last occurrence should be taken into account.
> 
> 
> > 
> > From page 59 (page 65 of the PDF) of the GLSL 4.40 spec:
> > 
> >   " More than one layout qualifier may appear in a single
> >     declaration. Additionally, the same layout-qualifier-name can
> >     occur multiple times within a layout qualifier or across multiple
> >     layout qualifiers in the same declaration. When the same
> >     layout-qualifier-name occurs multiple times, in a single
> >     declaration, the last occurrence overrides the former
> >     occurrence(s)."
> > 
> > Consider this example:
> > 
> >   " #version 150
> >     #extension GL_ARB_shading_language_420pack: enable
> >     #extension GL_ARB_enhanced_layouts: enable
> > 
> >     layout(max_vertices=2, 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.
> 
> 
> How about simply:
> 
> Although different values for "max_vertices" results in a compilation
> error. The above code is valid because max_vertices=2 is ignored.
> 
> > 
> > Hence, when merging qualifiers in an ast_type_qualifier, we now
> > ignore
> > new appearances of a same layout-qualifier-name if the
> > "is_single_layout_merge" parameter is on, since the GLSL parser works
> > in this case from right to left.
> 
> 
> When merging qualifiers in an ast_type_qualifier, we now simply ignore
> new appearances of a same layout-qualifier-name if the
> "is_single_layout_merge" parameter is true, this works because the GLSL
> parser processes qualifiers from right to left.
> 
> With those changes this patch is:
> 
> Reviewed-by: Timothy Arceri <timothy.arceri at collabora.com>
> 
> > 
> > Signed-off-by: Andres Gomez <agomez at igalia.com>
> > ---
> >  src/compiler/glsl/ast_type.cpp | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/ast_type.cpp
> > b/src/compiler/glsl/ast_type.cpp
> > index b586f94..504b533 100644
> > --- a/src/compiler/glsl/ast_type.cpp
> > +++ b/src/compiler/glsl/ast_type.cpp
> > @@ -196,7 +196,7 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> >     }
> >  
> >     if (q.flags.q.max_vertices) {
> > -      if (this->max_vertices) {
> > +      if (this->max_vertices && !is_single_layout_merge) {
> >           this->max_vertices->merge_qualifier(q.max_vertices);
> >        } else {
> >           this->max_vertices = q.max_vertices;
> > @@ -213,7 +213,7 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> >     }
> >  
> >     if (q.flags.q.invocations) {
> > -      if (this->invocations) {
> > +      if (this->invocations && !is_single_layout_merge) {
> >           this->invocations->merge_qualifier(q.invocations);
> >        } else {
> >           this->invocations = q.invocations;
> > @@ -262,7 +262,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> >           unsigned buff_idx;
> >           if (process_qualifier_constant(state, loc, "xfb_buffer",
> >                                          this->xfb_buffer,
> > &buff_idx)) {
> > -            if (state->out_qualifier->out_xfb_stride[buff_idx]) {
> > +            if (state->out_qualifier->out_xfb_stride[buff_idx]
> > +                && !is_single_layout_merge) {
> >                 state->out_qualifier->out_xfb_stride[buff_idx]-
> > > merge_qualifier(
> > 
> >                    new(state) ast_layout_expression(*loc, this-
> > > xfb_stride));
> > 
> >              } else {
> > @@ -274,7 +275,7 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> >     }
> >  
> >     if (q.flags.q.vertices) {
> > -      if (this->vertices) {
> > +      if (this->vertices && !is_single_layout_merge) {
> >           this->vertices->merge_qualifier(q.vertices);
> >        } else {
> >           this->vertices = q.vertices;
> > @@ -312,7 +313,7 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> >  
> >     for (int i = 0; i < 3; i++) {
> >        if (q.flags.q.local_size & (1 << i)) {
> > -         if (this->local_size[i]) {
> > +         if (this->local_size[i] && !is_single_layout_merge) {
> >              this->local_size[i]->merge_qualifier(q.local_size[i]);
> >           } else {
> >              this->local_size[i] = q.local_size[i];
> 
> 
> 
-- 
Br,

Andres