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

[Timothy Arceri]

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];