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 <ago...@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 > - ; _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev