[Mesa-dev] [PATCH 2/5] glsl: last duplicated layout-qualifier-name in multiple layout-qualifiers overrides the former
Timothy Arceri
timothy.arceri at collabora.com
Tue Oct 11 03:13:43 UTC 2016
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 <agomez at igalia.com>
> ---
> 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(&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.
> }
>
> 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 &q,
> - 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 &q,
> - 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;
> + }
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.
> +
> 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)
If you use merging_single_declaration as I suggest above this would
just be:
if (this->max_vertices && !merging_single_declaration)
> {
> this->max_vertices->merge_qualifier(q.max_vertices);
> } else {
> this->max_vertices = q.max_vertices;
> @@ -213,7 +226,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> }
>
> if (q.flags.q.invocations) {
> - if (this->invocations && !is_single_layout_merge) {
> + if (this->invocations
> + && !is_single_layout_merge && !is_multiple_layouts_merge)
> {
> this->invocations->merge_qualifier(q.invocations);
> } else {
> this->invocations = q.invocations;
> @@ -263,7 +277,7 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> if (process_qualifier_constant(state, loc, "xfb_buffer",
> this->xfb_buffer,
> &buff_idx)) {
> if (state->out_qualifier->out_xfb_stride[buff_idx]
> - && !is_single_layout_merge) {
> + && !is_single_layout_merge &&
> !is_multiple_layouts_merge) {
> state->out_qualifier->out_xfb_stride[buff_idx]-
> >merge_qualifier(
> new(state) ast_layout_expression(*loc, this-
> >xfb_stride));
> } else {
> @@ -275,7 +289,8 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> }
>
> if (q.flags.q.vertices) {
> - if (this->vertices && !is_single_layout_merge) {
> + if (this->vertices
> + && !is_single_layout_merge && !is_multiple_layouts_merge)
> {
> this->vertices->merge_qualifier(q.vertices);
> } else {
> this->vertices = q.vertices;
> @@ -313,7 +328,8 @@ 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] && !is_single_layout_merge) {
> + if (this->local_size[i]
> + && !is_single_layout_merge &&
> !is_multiple_layouts_merge) {
> this->local_size[i]->merge_qualifier(q.local_size[i]);
> } else {
> this->local_size[i] = q.local_size[i];
> 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
> @@ -297,10 +297,10 @@ static bool match_layout_qualifier(const char
> *s1, const char *s2,
> %type <node> for_init_statement
> %type <for_rest_statement> for_rest_statement
> %type <node> layout_defaults
> -%type <node> layout_uniform_defaults
> -%type <node> layout_buffer_defaults
> -%type <node> layout_in_defaults
> -%type <node> layout_out_defaults
> +%type <type_qualifier> layout_uniform_defaults
> +%type <type_qualifier> layout_buffer_defaults
> +%type <type_qualifier> layout_in_defaults
> +%type <type_qualifier> layout_out_defaults
>
> %right THEN ELSE
> %%
> @@ -1862,11 +1862,8 @@ type_qualifier:
> * precise qualifiers since these are useful in
> ARB_separate_shader_objects.
> * There is no clear spec guidance on this either.
> */
> - if (!state->has_420pack_or_es31() && $2.has_layout())
> - _mesa_glsl_error(&@1, state, "duplicate layout(...)
> qualifiers");
> -
> $$ = $1;
> - $$.merge_qualifier(&@1, state, $2, false);
> + $$.merge_qualifier(&@1, state, $2, false, $2.has_layout());
> }
> | subroutine_qualifier type_qualifier
> {
> @@ -2688,13 +2685,9 @@ interface_block:
> {
> ast_interface_block *block = (ast_interface_block *) $2;
>
> - if (!state->has_420pack_or_es31() && block-
> >layout.has_layout() &&
> - !block->layout.is_default_qualifier) {
> - _mesa_glsl_error(&@1, state, "duplicate layout(...)
> qualifiers");
> - YYERROR;
> - }
> -
> - if (!block->layout.merge_qualifier(& @1, state, $1, false)) {
> + if (!block->layout.merge_qualifier(& @1, state, $1, false,
> + block->layout.has_layout()
> &&
> + !block-
> >layout.is_default_qualifier)) {
> YYERROR;
> }
>
> @@ -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?
> + ;
> +
> +layout_buffer_defaults:
> + layout_qualifier layout_buffer_defaults
> {
> - if (!state->default_uniform_qualifier->
> - merge_qualifier(& @1, state, $1, false)) {
> + $$ = $1;
> + if (!$$.merge_qualifier(& @2, state, $2, false, true)) {
> YYERROR;
> }
> - $$ = NULL;
> }
> + | layout_qualifier BUFFER ';'
> ;
>
> -layout_buffer_defaults:
> - layout_qualifier layout_buffer_defaults
> +layout_in_defaults:
> + layout_qualifier layout_in_defaults
> + {
> + $$ = $1;
> + if (!$$.merge_qualifier(& @2, state, $2, false, true)) {
> + YYERROR;
> + }
> + }
> + | layout_qualifier IN_TOK ';'
> + ;
> +
> +layout_out_defaults:
> + layout_qualifier layout_out_defaults
> + {
> + $$ = $1;
> + if (!$$.merge_qualifier(& @2, state, $2, false, true)) {
> + YYERROR;
> + }
> + }
> + | layout_qualifier OUT_TOK ';'
> + ;
> +
> +layout_defaults:
> + layout_uniform_defaults
> {
> $$ = NULL;
> - if (!state->has_420pack_or_es31()) {
> - _mesa_glsl_error(&@1, state, "duplicate layout(...)
> qualifiers");
> + if (!state->default_uniform_qualifier->
> + merge_qualifier(& @1, state, $1, false)) {
> YYERROR;
> - } else {
> - if (!state->default_shader_storage_qualifier->
> - merge_qualifier(& @1, state, $1, false)) {
> - YYERROR;
> - }
> }
> }
> - | layout_qualifier BUFFER ';'
> + | layout_buffer_defaults
> {
> + $$ = NULL;
> if (!state->default_shader_storage_qualifier->
> merge_qualifier(& @1, state, $1, false)) {
> YYERROR;
> @@ -2879,27 +2888,8 @@ layout_buffer_defaults:
> _mesa_glsl_error(& @1, state,
> "binding qualifier cannot be set for
> default layout");
> }
> -
> - $$ = NULL;
> }
> - ;
> -
> -layout_in_defaults:
> - layout_qualifier layout_in_defaults
> - {
> - $$ = NULL;
> - if (!state->has_420pack_or_es31()) {
> - _mesa_glsl_error(&@1, state, "duplicate layout(...)
> qualifiers");
> - YYERROR;
> - } else {
> - if (!state->in_qualifier->
> - merge_in_qualifier(& @1, state, $1, $$, false)) {
> - YYERROR;
> - }
> - $$ = $2;
> - }
> - }
> - | layout_qualifier IN_TOK ';'
> + | layout_in_defaults
> {
> $$ = NULL;
> if (!state->in_qualifier->
> @@ -2907,35 +2897,12 @@ layout_in_defaults:
> YYERROR;
> }
> }
> - ;
> -
> -layout_out_defaults:
> - layout_qualifier layout_out_defaults
> - {
> - $$ = NULL;
> - if (!state->has_420pack_or_es31()) {
> - _mesa_glsl_error(&@1, state, "duplicate layout(...)
> qualifiers");
> - YYERROR;
> - } else {
> - if (!state->out_qualifier->
> - merge_out_qualifier(& @1, state, $1, $$, false)) {
> - YYERROR;
> - }
> - $$ = $2;
> - }
> - }
> - | layout_qualifier OUT_TOK ';'
> + | layout_out_defaults
> {
> $$ = NULL;
> if (!state->out_qualifier->
> - merge_out_qualifier(& @1, state, $1, $$, true))
> + merge_out_qualifier(& @1, state, $1, $$, true)) {
> YYERROR;
> + }
> }
> ;
> -
> -layout_defaults:
> - layout_uniform_defaults
> - | layout_buffer_defaults
> - | layout_in_defaults
> - | layout_out_defaults
> - ;
More information about the mesa-dev
mailing list