[Mesa-dev] [PATCH v2 0/9] deal with multiple appearances of the same layout-qualifier-name in a single declaration

[Timothy Arceri]

On Sat, 2016-10-22 at 23:09 +0300, Andres Gomez wrote:
> In the case of layout-qualifier-names that can appear multiple times
> in different declarations of the same shader or, even, the same
> program, but that have to consistently hold the same value we are
> using the ast_layout_expression class which holds a list to store all
> the appearances to be able to check for coherence later.
> 
> Until now, we were also holding inside the ast_layout_expression
> values of the same layout-qualifier-name that could appear inside a
> single layout-qualifier or across multiple layout-qualifiers in a
> single declaration.
> 
> This was a problem since, inside a declaration, only the last
> appearance should be taken into account. As we were not doing this,
> the compilation or linking was failing due to different values of the
> same layout-qualifier-name in a single declaration when such
> layout-qualifier-name had as a constraint to hold the same value
> across the same shader or program.
> 
> Now, we only hold the last appearanace of a repeated
> layout-qualifier-name inside a single declaration.
> 
> These following 2 example will help to illustrate the problem:
> 
> - " #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;"
> 
> - " #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 "max_vertices" should result in a
> compilation error. The above code is valid because max_vertices=2 is
> ignored.
> 
> In addition, the series includes changes to allow multiple
> layout-qualifiers in the same declaration if
> ARB_shading_language_420pack *or ARB_enhanced_layouts* are supported.

As mentioned in the piglit test reviews I'm not so sure this is
correct.

> 
> This change is not 100% clear to me since the addition in
> ARB_enhanced_layouts is for a duplicated layout-qualifier-name in a
> single layout-qualifier but it seems to make sense since the change
> builds up on the precondition that multiple layout-qualifiers are
> already allowed in a single declaration.
> 
> The main changes in this v2 series are:
>   Patch 2/9 is new and fixes a related problem in Interface Blocks
>     that was detected when developing piglit tests for the original
>     series.
>   Patches 3/9 and 4/9 are split from the patch 2/5 of the original
>     series.
>   Patch 5/9 is new and allows multiple layout-qualifiers in the same
>     declaration just if the ARB_enhanced_layouts spec is supported.
>   Patches 7/9 and 8/9 replace patch 4/5 from the original series.
> 
> Fixes:
> - GL44-CTS.shading_language_420pack.qualifier_override_layout
> - GL44-CTS.enhanced_layouts.xfb_duplicated_stride
> 
> Andres Gomez (9):
>   glsl: ignore all but the rightmost layout-qualifier-name
>   glsl: merge layouts into the default one as the last step in
> interface
>     blocks
>   glsl: ignore all but the rightmost layout qualifier name from the
>     rightmost layout qualifier
>   glsl: simplified error checking for duplicated layout-qualifiers
>   glsl: allow multiple layout-qualifier in single declaration if
>     enhanced layouts
>   glsl: push layout-qualifier-name values from variable declarations
> to
>     global
>   Revert "glsl: geom shader max_vertices layout must match."
>   Revert "glsl: allow layout qualifier overrides with
>     ARB_shading_language_420pack"
>   glsl: simplified ast_type_qualifier::merge_[in|out]_qualifier API
> 
>  src/compiler/glsl/ast.h                  |  25 +++--
>  src/compiler/glsl/ast_type.cpp           | 127 ++++++++++++++-------
> ---
>  src/compiler/glsl/glsl_parser.yy         | 165 +++++++++++++++----
> ------------
>  src/compiler/glsl/glsl_parser_extras.cpp |   4 +-
>  4 files changed, 173 insertions(+), 148 deletions(-)
>