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.arc...@collabora.com>
>
> >
> > Signed-off-by: Andres Gomez <ago...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
