[Mesa-dev,01/18] glsl: Add parsing support for multi-stream output in geometry shaders.

Submitted by Iago Toral Quiroga on June 11, 2014, 7:49 a.m.

Details

Message ID 1402472981-22428-2-git-send-email-itoral@igalia.com
State Superseded
Headers show

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga June 11, 2014, 7:49 a.m.
From: Samuel Iglesias Gonsalvez <siglesias@igalia.com>

This implements parsing requirements for multi-stream support in
geometry shaders as defined in ARB_gpu_shader5.

Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
---
 src/glsl/ast.h          |  5 +++++
 src/glsl/ast_to_hir.cpp |  6 ++++++
 src/glsl/ast_type.cpp   | 19 ++++++++++++++++++-
 src/glsl/glsl_parser.yy | 45 +++++++++++++++++++++++++++++++++++++++++++++
 src/glsl/ir.h           |  5 +++++
 5 files changed, 79 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index 56e7bd8..823b1d2 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -509,6 +509,8 @@  struct ast_type_qualifier {
          /** \name Layout qualifiers for GL_ARB_gpu_shader5 */
          /** \{ */
          unsigned invocations:1;
+         unsigned streamId:1; /* Has streamId value assigned  */
+         unsigned explicit_streamId:1; /* streamId value assigned explicitely by shader code */
          /** \} */
       }
       /** \brief Set of flags, accessed by name. */
@@ -542,6 +544,9 @@  struct ast_type_qualifier {
    /** Maximum output vertices in GLSL 1.50 geometry shaders. */
    int max_vertices;
 
+   /** Stream ID in GLSL 1.50 geometry shaders. */
+   unsigned streamId;
+
    /** Input or output primitive type in GLSL 1.50 geometry shaders */
    GLenum prim_type;
 
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
index 140bb74..ab0f50f 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -2426,6 +2426,11 @@  apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
    if (qual->flags.q.sample)
       var->data.sample = 1;
 
+   if (state->stage == MESA_SHADER_GEOMETRY &&
+       qual->flags.q.out && qual->flags.q.streamId) {
+      var->data.streamId = qual->streamId;
+   }
+
    if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
       var->type = glsl_type::error_type;
       _mesa_glsl_error(loc, state,
@@ -5455,6 +5460,7 @@  ast_interface_block::hir(exec_list *instructions,
          var->data.centroid = fields[i].centroid;
          var->data.sample = fields[i].sample;
          var->init_interface_type(block_type);
+         var->data.streamId = this->layout.streamId;
 
          if (redeclaring_per_vertex) {
             ir_variable *earlier =
diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
index 0ee2c49..749f161 100644
--- a/src/glsl/ast_type.cpp
+++ b/src/glsl/ast_type.cpp
@@ -125,9 +125,13 @@  ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
    /* Uniform block layout qualifiers get to overwrite each
     * other (rightmost having priority), while all other
     * qualifiers currently don't allow duplicates.
+    *
+    * Geometry shaders can have several layout qualifiers
+    * assigning different stream ID values.
     */
 
-   if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
+   if ((state->stage != MESA_SHADER_GEOMETRY) &&
+       (this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
 				      ubo_layout_mask.flags.i |
                                       ubo_binding_mask.flags.i)) != 0) {
       _mesa_glsl_error(loc, state,
@@ -154,6 +158,19 @@  ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
       this->max_vertices = q.max_vertices;
    }
 
+   if (state->stage == MESA_SHADER_GEOMETRY &&
+       state->ARB_gpu_shader5_enable &&
+       !this->flags.q.explicit_streamId) {
+      if (q.flags.q.streamId) {
+         this->flags.q.streamId = 1;
+         this->streamId = q.streamId;
+      } else if(this->flags.q.streamId == 0) {
+         /* Assign default global streamId value */
+         this->flags.q.streamId = 1;
+         this->streamId = state->out_qualifier->streamId;
+      }
+   }
+
    if ((q.flags.i & ubo_mat_mask.flags.i) != 0)
       this->flags.i &= ~ubo_mat_mask.flags.i;
    if ((q.flags.i & ubo_layout_mask.flags.i) != 0)
diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
index eddab05..caebfe7 100644
--- a/src/glsl/glsl_parser.yy
+++ b/src/glsl/glsl_parser.yy
@@ -1395,6 +1395,21 @@  layout_qualifier_id:
          }
       }
 
+      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {
+         if (match_layout_qualifier("stream", $1, state) == 0) {
+            $$.flags.q.streamId = 1;
+
+            if (($3 < 0) || ($3 >= MAX_VERTEX_STREAMS)) {
+               _mesa_glsl_error(& @3, state,
+                                "invalid stream %d specified", $3);
+               YYERROR;
+            } else {
+               $$.flags.q.explicit_streamId = 1;
+               $$.streamId = $3;
+            }
+         }
+      }
+
       static const char * const local_size_qualifiers[3] = {
          "local_size_x",
          "local_size_y",
@@ -1713,6 +1728,16 @@  storage_qualifier:
    {
       memset(& $$, 0, sizeof($$));
       $$.flags.q.out = 1;
+
+      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {
+         /*
+          * According to the GL_ARB_gpu_shader5 spec, assign a default value
+          * to the streamId
+          */
+          $$.flags.q.streamId = 1;
+          $$.flags.q.explicit_streamId = 0;
+          $$.streamId = state->out_qualifier->streamId;
+      }
    }
    | UNIFORM
    {
@@ -2381,6 +2406,18 @@  interface_block:
       if (!block->layout.merge_qualifier(& @1, state, $1)) {
          YYERROR;
       }
+
+      foreach_list_typed (ast_declarator_list, member, link, &block->declarations) {
+         ast_type_qualifier& qualifier = member->type->qualifier;
+         if (qualifier.flags.q.streamId && qualifier.streamId != block->layout.streamId) {
+               _mesa_glsl_error(& @1, state,
+                             "stream layout qualifier on "
+                             "interface block member does not match "
+                             "the interface block (%d and %d)",
+                             qualifier.streamId, block->layout.streamId);
+               YYERROR;
+         }
+      }
       $$ = block;
    }
    ;
@@ -2454,6 +2491,12 @@  basic_interface_block:
 
       block->layout.flags.i |= block_interface_qualifier;
 
+      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {
+         /* Assign global layout's streamId value. */
+         block->layout.flags.q.streamId = 1;
+         block->layout.streamId = state->out_qualifier->streamId;
+      }
+
       foreach_list_typed (ast_declarator_list, member, link, &block->declarations) {
          ast_type_qualifier& qualifier = member->type->qualifier;
          if ((qualifier.flags.i & interface_type_mask) == 0) {
@@ -2596,6 +2639,8 @@  layout_defaults:
          }
          if (!state->out_qualifier->merge_qualifier(& @1, state, $1))
             YYERROR;
+         /* Allow future assigments of global out's stream id value */
+         state->out_qualifier->flags.q.explicit_streamId = 0;
       }
       $$ = NULL;
    }
diff --git a/src/glsl/ir.h b/src/glsl/ir.h
index b4e52d3..8cc58af 100644
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -705,6 +705,11 @@  public:
        */
       int location;
 
+      /*
+       * Vertex stream output identifier.
+       */
+      unsigned streamId;
+
       /**
        * output index for dual source blending.
        */

Comments

On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga <itoral@igalia.com> wrote:
> @@ -509,6 +509,8 @@ struct ast_type_qualifier {
>           /** \name Layout qualifiers for GL_ARB_gpu_shader5 */
>           /** \{ */
>           unsigned invocations:1;
> +         unsigned streamId:1; /* Has streamId value assigned  */
> +         unsigned explicit_streamId:1; /* streamId value assigned explicitely by shader code */

s/explicitely/explicitly/

I'd call these stream / explicit_stream (or if you must, stream_id /
explicit_stream_id). None of the other members here are using camel
case, and explicit_streamId is a crazy mix.

>           /** \} */
>        }
>        /** \brief Set of flags, accessed by name. */
> @@ -542,6 +544,9 @@ struct ast_type_qualifier {
>     /** Maximum output vertices in GLSL 1.50 geometry shaders. */
>     int max_vertices;
>
> +   /** Stream ID in GLSL 1.50 geometry shaders. */
> +   unsigned streamId;
> +
>     /** Input or output primitive type in GLSL 1.50 geometry shaders */
>     GLenum prim_type;
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 140bb74..ab0f50f 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2426,6 +2426,11 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>     if (qual->flags.q.sample)
>        var->data.sample = 1;
>
> +   if (state->stage == MESA_SHADER_GEOMETRY &&
> +       qual->flags.q.out && qual->flags.q.streamId) {
> +      var->data.streamId = qual->streamId;
> +   }
> +
>     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
>        var->type = glsl_type::error_type;
>        _mesa_glsl_error(loc, state,
> @@ -5455,6 +5460,7 @@ ast_interface_block::hir(exec_list *instructions,
>           var->data.centroid = fields[i].centroid;
>           var->data.sample = fields[i].sample;
>           var->init_interface_type(block_type);
> +         var->data.streamId = this->layout.streamId;
>
>           if (redeclaring_per_vertex) {
>              ir_variable *earlier =
> diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
> index 0ee2c49..749f161 100644
> --- a/src/glsl/ast_type.cpp
> +++ b/src/glsl/ast_type.cpp
> @@ -125,9 +125,13 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>     /* Uniform block layout qualifiers get to overwrite each
>      * other (rightmost having priority), while all other
>      * qualifiers currently don't allow duplicates.
> +    *
> +    * Geometry shaders can have several layout qualifiers
> +    * assigning different stream ID values.
>      */
>
> -   if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
> +   if ((state->stage != MESA_SHADER_GEOMETRY) &&
> +       (this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
>                                       ubo_layout_mask.flags.i |
>                                        ubo_binding_mask.flags.i)) != 0) {
>        _mesa_glsl_error(loc, state,
> @@ -154,6 +158,19 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>        this->max_vertices = q.max_vertices;
>     }
>
> +   if (state->stage == MESA_SHADER_GEOMETRY &&
> +       state->ARB_gpu_shader5_enable &&
> +       !this->flags.q.explicit_streamId) {
> +      if (q.flags.q.streamId) {
> +         this->flags.q.streamId = 1;
> +         this->streamId = q.streamId;
> +      } else if(this->flags.q.streamId == 0) {

!this->flags.q.streamId, for consistency with the explicit_streamId check above.

> +         /* Assign default global streamId value */
> +         this->flags.q.streamId = 1;
> +         this->streamId = state->out_qualifier->streamId;
> +      }
> +   }
> +
>     if ((q.flags.i & ubo_mat_mask.flags.i) != 0)
>        this->flags.i &= ~ubo_mat_mask.flags.i;
>     if ((q.flags.i & ubo_layout_mask.flags.i) != 0)
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index eddab05..caebfe7 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -1395,6 +1395,21 @@ layout_qualifier_id:
>           }
>        }
>
> +      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {

This check (and others like it) should really also pass if
state->is_version(400, 0) as well, so we
don't have to come back and change them all again.

> +         if (match_layout_qualifier("stream", $1, state) == 0) {
> +            $$.flags.q.streamId = 1;
> +
> +            if (($3 < 0) || ($3 >= MAX_VERTEX_STREAMS)) {

This should really be checking against the real limit... but I see
there's a precedent for this in the
equivalent MAX_GEOMETRY_SHADER_INVOCATIONS case, which is much more
likely to vary
across hardware...

> +               _mesa_glsl_error(& @3, state,
> +                                "invalid stream %d specified", $3);
> +               YYERROR;
> +            } else {
> +               $$.flags.q.explicit_streamId = 1;
> +               $$.streamId = $3;
> +            }
> +         }
> +      }
> +

> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index b4e52d3..8cc58af 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -705,6 +705,11 @@ public:
>         */
>        int location;
>
> +      /*
> +       * Vertex stream output identifier.
> +       */
> +      unsigned streamId;

Same comment as in the ast about naming.

> +
>        /**
>         * output index for dual source blending.
>         */
> --
> 1.9.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 06/11/2014 01:40 AM, Chris Forbes wrote:
> On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga <itoral@igalia.com> wrote:
>> @@ -509,6 +509,8 @@ struct ast_type_qualifier {
>>           /** \name Layout qualifiers for GL_ARB_gpu_shader5 */
>>           /** \{ */
>>           unsigned invocations:1;
>> +         unsigned streamId:1; /* Has streamId value assigned  */
>> +         unsigned explicit_streamId:1; /* streamId value assigned explicitely by shader code */
> 
> s/explicitely/explicitly/
> 
> I'd call these stream / explicit_stream (or if you must, stream_id /
> explicit_stream_id). None of the other members here are using camel
> case, and explicit_streamId is a crazy mix.

I agree.

>>           /** \} */
>>        }
>>        /** \brief Set of flags, accessed by name. */
>> @@ -542,6 +544,9 @@ struct ast_type_qualifier {
>>     /** Maximum output vertices in GLSL 1.50 geometry shaders. */
>>     int max_vertices;
>>
>> +   /** Stream ID in GLSL 1.50 geometry shaders. */
>> +   unsigned streamId;
>> +
>>     /** Input or output primitive type in GLSL 1.50 geometry shaders */
>>     GLenum prim_type;
>>
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index 140bb74..ab0f50f 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -2426,6 +2426,11 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>>     if (qual->flags.q.sample)
>>        var->data.sample = 1;
>>
>> +   if (state->stage == MESA_SHADER_GEOMETRY &&
>> +       qual->flags.q.out && qual->flags.q.streamId) {
>> +      var->data.streamId = qual->streamId;
>> +   }
>> +
>>     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
>>        var->type = glsl_type::error_type;
>>        _mesa_glsl_error(loc, state,
>> @@ -5455,6 +5460,7 @@ ast_interface_block::hir(exec_list *instructions,
>>           var->data.centroid = fields[i].centroid;
>>           var->data.sample = fields[i].sample;
>>           var->init_interface_type(block_type);
>> +         var->data.streamId = this->layout.streamId;
>>
>>           if (redeclaring_per_vertex) {
>>              ir_variable *earlier =
>> diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
>> index 0ee2c49..749f161 100644
>> --- a/src/glsl/ast_type.cpp
>> +++ b/src/glsl/ast_type.cpp
>> @@ -125,9 +125,13 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>>     /* Uniform block layout qualifiers get to overwrite each
>>      * other (rightmost having priority), while all other
>>      * qualifiers currently don't allow duplicates.
>> +    *
>> +    * Geometry shaders can have several layout qualifiers
>> +    * assigning different stream ID values.
>>      */
>>
>> -   if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
>> +   if ((state->stage != MESA_SHADER_GEOMETRY) &&
>> +       (this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
>>                                       ubo_layout_mask.flags.i |
>>                                        ubo_binding_mask.flags.i)) != 0) {
>>        _mesa_glsl_error(loc, state,
>> @@ -154,6 +158,19 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>>        this->max_vertices = q.max_vertices;
>>     }
>>
>> +   if (state->stage == MESA_SHADER_GEOMETRY &&
>> +       state->ARB_gpu_shader5_enable &&
>> +       !this->flags.q.explicit_streamId) {
>> +      if (q.flags.q.streamId) {
>> +         this->flags.q.streamId = 1;
>> +         this->streamId = q.streamId;
>> +      } else if(this->flags.q.streamId == 0) {
> 
> !this->flags.q.streamId, for consistency with the explicit_streamId check above.
> 
>> +         /* Assign default global streamId value */
>> +         this->flags.q.streamId = 1;
>> +         this->streamId = state->out_qualifier->streamId;
>> +      }
>> +   }
>> +
>>     if ((q.flags.i & ubo_mat_mask.flags.i) != 0)
>>        this->flags.i &= ~ubo_mat_mask.flags.i;
>>     if ((q.flags.i & ubo_layout_mask.flags.i) != 0)
>> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
>> index eddab05..caebfe7 100644
>> --- a/src/glsl/glsl_parser.yy
>> +++ b/src/glsl/glsl_parser.yy
>> @@ -1395,6 +1395,21 @@ layout_qualifier_id:
>>           }
>>        }
>>
>> +      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {
> 
> This check (and others like it) should really also pass if
> state->is_version(400, 0) as well, so we
> don't have to come back and change them all again.

In similar situations we add a method like check_explicit_stream_allowed
that encapsulates the various checks.  See
_mesa_glsl_parser_state::check_explicit_attrib_location_allowed (in
glsl_parser_extras.h).

>> +         if (match_layout_qualifier("stream", $1, state) == 0) {
>> +            $$.flags.q.streamId = 1;
>> +
>> +            if (($3 < 0) || ($3 >= MAX_VERTEX_STREAMS)) {
> 
> This should really be checking against the real limit... but I see
> there's a precedent for this in the
> equivalent MAX_GEOMETRY_SHADER_INVOCATIONS case, which is much more
> likely to vary
> across hardware...

If there is a hard, upper limit, we may check against that in the
parser.  Sometimes we store the value in a small bitfield, so checking
against the hard limit prevents trying to store 0xDEADBEEF in 3 bits or
something. :)

>> +               _mesa_glsl_error(& @3, state,
>> +                                "invalid stream %d specified", $3);
>> +               YYERROR;
>> +            } else {
>> +               $$.flags.q.explicit_streamId = 1;
>> +               $$.streamId = $3;
>> +            }
>> +         }
>> +      }
>> +
> 
>> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>> index b4e52d3..8cc58af 100644
>> --- a/src/glsl/ir.h
>> +++ b/src/glsl/ir.h
>> @@ -705,6 +705,11 @@ public:
>>         */
>>        int location;
>>
>> +      /*
>> +       * Vertex stream output identifier.
>> +       */
>> +      unsigned streamId;
> 
> Same comment as in the ast about naming.
> 
>> +
>>        /**
>>         * output index for dual source blending.
>>         */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 06/11/2014 12:49 AM, Iago Toral Quiroga wrote:
> From: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> 
> This implements parsing requirements for multi-stream support in
> geometry shaders as defined in ARB_gpu_shader5.
> 
> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> ---
>  src/glsl/ast.h          |  5 +++++
>  src/glsl/ast_to_hir.cpp |  6 ++++++
>  src/glsl/ast_type.cpp   | 19 ++++++++++++++++++-
>  src/glsl/glsl_parser.yy | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  src/glsl/ir.h           |  5 +++++
>  5 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 56e7bd8..823b1d2 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -509,6 +509,8 @@ struct ast_type_qualifier {
>           /** \name Layout qualifiers for GL_ARB_gpu_shader5 */
>           /** \{ */
>           unsigned invocations:1;
> +         unsigned streamId:1; /* Has streamId value assigned  */
> +         unsigned explicit_streamId:1; /* streamId value assigned explicitely by shader code */
>           /** \} */
>        }
>        /** \brief Set of flags, accessed by name. */
> @@ -542,6 +544,9 @@ struct ast_type_qualifier {
>     /** Maximum output vertices in GLSL 1.50 geometry shaders. */
>     int max_vertices;
>  
> +   /** Stream ID in GLSL 1.50 geometry shaders. */
> +   unsigned streamId;
> +
>     /** Input or output primitive type in GLSL 1.50 geometry shaders */
>     GLenum prim_type;
>  
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 140bb74..ab0f50f 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2426,6 +2426,11 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>     if (qual->flags.q.sample)
>        var->data.sample = 1;
>  
> +   if (state->stage == MESA_SHADER_GEOMETRY &&
> +       qual->flags.q.out && qual->flags.q.streamId) {
> +      var->data.streamId = qual->streamId;
> +   }
> +
>     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
>        var->type = glsl_type::error_type;
>        _mesa_glsl_error(loc, state,
> @@ -5455,6 +5460,7 @@ ast_interface_block::hir(exec_list *instructions,
>           var->data.centroid = fields[i].centroid;
>           var->data.sample = fields[i].sample;
>           var->init_interface_type(block_type);
> +         var->data.streamId = this->layout.streamId;
>  
>           if (redeclaring_per_vertex) {
>              ir_variable *earlier =
> diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
> index 0ee2c49..749f161 100644
> --- a/src/glsl/ast_type.cpp
> +++ b/src/glsl/ast_type.cpp
> @@ -125,9 +125,13 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>     /* Uniform block layout qualifiers get to overwrite each
>      * other (rightmost having priority), while all other
>      * qualifiers currently don't allow duplicates.
> +    *
> +    * Geometry shaders can have several layout qualifiers
> +    * assigning different stream ID values.
>      */
>  
> -   if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
> +   if ((state->stage != MESA_SHADER_GEOMETRY) &&
> +       (this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
>  				      ubo_layout_mask.flags.i |
>                                        ubo_binding_mask.flags.i)) != 0) {

I think this will allow multiply layout(location=) qualifiers on
geometry shader inputs / outputs.  So...

#version 150
#extension GL_ARB_separate_shader_objects: require

layout(location=1) layout(location=2) in vec4 v;

should generate an error, but I think this change will disable that error.

>        _mesa_glsl_error(loc, state,
> @@ -154,6 +158,19 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>        this->max_vertices = q.max_vertices;
>     }
>  
> +   if (state->stage == MESA_SHADER_GEOMETRY &&
> +       state->ARB_gpu_shader5_enable &&
> +       !this->flags.q.explicit_streamId) {
> +      if (q.flags.q.streamId) {
> +         this->flags.q.streamId = 1;
> +         this->streamId = q.streamId;
> +      } else if(this->flags.q.streamId == 0) {
> +         /* Assign default global streamId value */
> +         this->flags.q.streamId = 1;
> +         this->streamId = state->out_qualifier->streamId;
> +      }
> +   }
> +
>     if ((q.flags.i & ubo_mat_mask.flags.i) != 0)
>        this->flags.i &= ~ubo_mat_mask.flags.i;
>     if ((q.flags.i & ubo_layout_mask.flags.i) != 0)
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index eddab05..caebfe7 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -1395,6 +1395,21 @@ layout_qualifier_id:
>           }
>        }
>  
> +      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {
> +         if (match_layout_qualifier("stream", $1, state) == 0) {
> +            $$.flags.q.streamId = 1;
> +
> +            if (($3 < 0) || ($3 >= MAX_VERTEX_STREAMS)) {
> +               _mesa_glsl_error(& @3, state,
> +                                "invalid stream %d specified", $3);
> +               YYERROR;
> +            } else {
> +               $$.flags.q.explicit_streamId = 1;
> +               $$.streamId = $3;
> +            }
> +         }
> +      }
> +
>        static const char * const local_size_qualifiers[3] = {
>           "local_size_x",
>           "local_size_y",
> @@ -1713,6 +1728,16 @@ storage_qualifier:
>     {
>        memset(& $$, 0, sizeof($$));
>        $$.flags.q.out = 1;
> +
> +      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {
> +         /*
> +          * According to the GL_ARB_gpu_shader5 spec, assign a default value
> +          * to the streamId
> +          */
> +          $$.flags.q.streamId = 1;
> +          $$.flags.q.explicit_streamId = 0;
> +          $$.streamId = state->out_qualifier->streamId;
> +      }
>     }
>     | UNIFORM
>     {
> @@ -2381,6 +2406,18 @@ interface_block:
>        if (!block->layout.merge_qualifier(& @1, state, $1)) {
>           YYERROR;
>        }
> +
> +      foreach_list_typed (ast_declarator_list, member, link, &block->declarations) {
> +         ast_type_qualifier& qualifier = member->type->qualifier;
> +         if (qualifier.flags.q.streamId && qualifier.streamId != block->layout.streamId) {
> +               _mesa_glsl_error(& @1, state,
> +                             "stream layout qualifier on "
> +                             "interface block member does not match "
> +                             "the interface block (%d and %d)",
> +                             qualifier.streamId, block->layout.streamId);
> +               YYERROR;
> +         }
> +      }
>        $$ = block;
>     }
>     ;
> @@ -2454,6 +2491,12 @@ basic_interface_block:
>  
>        block->layout.flags.i |= block_interface_qualifier;
>  
> +      if (state->ARB_gpu_shader5_enable && state->stage == MESA_SHADER_GEOMETRY) {
> +         /* Assign global layout's streamId value. */
> +         block->layout.flags.q.streamId = 1;
> +         block->layout.streamId = state->out_qualifier->streamId;
> +      }
> +
>        foreach_list_typed (ast_declarator_list, member, link, &block->declarations) {
>           ast_type_qualifier& qualifier = member->type->qualifier;
>           if ((qualifier.flags.i & interface_type_mask) == 0) {
> @@ -2596,6 +2639,8 @@ layout_defaults:
>           }
>           if (!state->out_qualifier->merge_qualifier(& @1, state, $1))
>              YYERROR;
> +         /* Allow future assigments of global out's stream id value */
> +         state->out_qualifier->flags.q.explicit_streamId = 0;
>        }
>        $$ = NULL;
>     }
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index b4e52d3..8cc58af 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -705,6 +705,11 @@ public:
>         */
>        int location;
>  
> +      /*
> +       * Vertex stream output identifier.
> +       */
> +      unsigned streamId;
> +
>        /**
>         * output index for dual source blending.
>         */
>
On Wed, 2014-06-11 at 15:29 -0700, Ian Romanick wrote:
> On 06/11/2014 12:49 AM, Iago Toral Quiroga wrote:
> > From: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> > 
> > This implements parsing requirements for multi-stream support in
> > geometry shaders as defined in ARB_gpu_shader5.
> > 
> > Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> > ---
> >  src/glsl/ast.h          |  5 +++++
> >  src/glsl/ast_to_hir.cpp |  6 ++++++
> >  src/glsl/ast_type.cpp   | 19 ++++++++++++++++++-
> >  src/glsl/glsl_parser.yy | 45 +++++++++++++++++++++++++++++++++++++++++++++
> >  src/glsl/ir.h           |  5 +++++
> >  5 files changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> > index 56e7bd8..823b1d2 100644
> > --- a/src/glsl/ast.h
> > +++ b/src/glsl/ast.h
> > @@ -509,6 +509,8 @@ struct ast_type_qualifier {
> >           /** \name Layout qualifiers for GL_ARB_gpu_shader5 */
> >           /** \{ */
> >           unsigned invocations:1;
> > +         unsigned streamId:1; /* Has streamId value assigned  */
> > +         unsigned explicit_streamId:1; /* streamId value assigned explicitely by shader code */
> >           /** \} */
> >        }
> >        /** \brief Set of flags, accessed by name. */
> > @@ -542,6 +544,9 @@ struct ast_type_qualifier {
> >     /** Maximum output vertices in GLSL 1.50 geometry shaders. */
> >     int max_vertices;
> >  
> > +   /** Stream ID in GLSL 1.50 geometry shaders. */
> > +   unsigned streamId;
> > +
> >     /** Input or output primitive type in GLSL 1.50 geometry shaders */
> >     GLenum prim_type;
> >  
> > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > index 140bb74..ab0f50f 100644
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
> > @@ -2426,6 +2426,11 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
> >     if (qual->flags.q.sample)
> >        var->data.sample = 1;
> >  
> > +   if (state->stage == MESA_SHADER_GEOMETRY &&
> > +       qual->flags.q.out && qual->flags.q.streamId) {
> > +      var->data.streamId = qual->streamId;
> > +   }
> > +
> >     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
> >        var->type = glsl_type::error_type;
> >        _mesa_glsl_error(loc, state,
> > @@ -5455,6 +5460,7 @@ ast_interface_block::hir(exec_list *instructions,
> >           var->data.centroid = fields[i].centroid;
> >           var->data.sample = fields[i].sample;
> >           var->init_interface_type(block_type);
> > +         var->data.streamId = this->layout.streamId;
> >  
> >           if (redeclaring_per_vertex) {
> >              ir_variable *earlier =
> > diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
> > index 0ee2c49..749f161 100644
> > --- a/src/glsl/ast_type.cpp
> > +++ b/src/glsl/ast_type.cpp
> > @@ -125,9 +125,13 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> >     /* Uniform block layout qualifiers get to overwrite each
> >      * other (rightmost having priority), while all other
> >      * qualifiers currently don't allow duplicates.
> > +    *
> > +    * Geometry shaders can have several layout qualifiers
> > +    * assigning different stream ID values.
> >      */
> >  
> > -   if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
> > +   if ((state->stage != MESA_SHADER_GEOMETRY) &&
> > +       (this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
> >  				      ubo_layout_mask.flags.i |
> >                                        ubo_binding_mask.flags.i)) != 0) {
> 
> I think this will allow multiply layout(location=) qualifiers on
> geometry shader inputs / outputs.  So...
> 
> #version 150
> #extension GL_ARB_separate_shader_objects: require
> 
> layout(location=1) layout(location=2) in vec4 v;
> 
> should generate an error, but I think this change will disable that error.
> 

OK, we will test it and modify the patch accordingly.

Thanks for the review!

Sam
On Thu, 2014-06-12 at 08:32 +0200, Samuel Iglesias Gonsálvez wrote:
> On Wed, 2014-06-11 at 15:29 -0700, Ian Romanick wrote:
> > On 06/11/2014 12:49 AM, Iago Toral Quiroga wrote:
> > > From: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> > > 
> > > This implements parsing requirements for multi-stream support in
> > > geometry shaders as defined in ARB_gpu_shader5.
> > > 
> > > Signed-off-by: Samuel Iglesias Gonsalvez <siglesias@igalia.com>
> > > ---
> > >  src/glsl/ast.h          |  5 +++++
> > >  src/glsl/ast_to_hir.cpp |  6 ++++++
> > >  src/glsl/ast_type.cpp   | 19 ++++++++++++++++++-
> > >  src/glsl/glsl_parser.yy | 45 +++++++++++++++++++++++++++++++++++++++++++++
> > >  src/glsl/ir.h           |  5 +++++
> > >  5 files changed, 79 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> > > index 56e7bd8..823b1d2 100644
> > > --- a/src/glsl/ast.h
> > > +++ b/src/glsl/ast.h
> > > @@ -509,6 +509,8 @@ struct ast_type_qualifier {
> > >           /** \name Layout qualifiers for GL_ARB_gpu_shader5 */
> > >           /** \{ */
> > >           unsigned invocations:1;
> > > +         unsigned streamId:1; /* Has streamId value assigned  */
> > > +         unsigned explicit_streamId:1; /* streamId value assigned explicitely by shader code */
> > >           /** \} */
> > >        }
> > >        /** \brief Set of flags, accessed by name. */
> > > @@ -542,6 +544,9 @@ struct ast_type_qualifier {
> > >     /** Maximum output vertices in GLSL 1.50 geometry shaders. */
> > >     int max_vertices;
> > >  
> > > +   /** Stream ID in GLSL 1.50 geometry shaders. */
> > > +   unsigned streamId;
> > > +
> > >     /** Input or output primitive type in GLSL 1.50 geometry shaders */
> > >     GLenum prim_type;
> > >  
> > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> > > index 140bb74..ab0f50f 100644
> > > --- a/src/glsl/ast_to_hir.cpp
> > > +++ b/src/glsl/ast_to_hir.cpp
> > > @@ -2426,6 +2426,11 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
> > >     if (qual->flags.q.sample)
> > >        var->data.sample = 1;
> > >  
> > > +   if (state->stage == MESA_SHADER_GEOMETRY &&
> > > +       qual->flags.q.out && qual->flags.q.streamId) {
> > > +      var->data.streamId = qual->streamId;
> > > +   }
> > > +
> > >     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
> > >        var->type = glsl_type::error_type;
> > >        _mesa_glsl_error(loc, state,
> > > @@ -5455,6 +5460,7 @@ ast_interface_block::hir(exec_list *instructions,
> > >           var->data.centroid = fields[i].centroid;
> > >           var->data.sample = fields[i].sample;
> > >           var->init_interface_type(block_type);
> > > +         var->data.streamId = this->layout.streamId;
> > >  
> > >           if (redeclaring_per_vertex) {
> > >              ir_variable *earlier =
> > > diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
> > > index 0ee2c49..749f161 100644
> > > --- a/src/glsl/ast_type.cpp
> > > +++ b/src/glsl/ast_type.cpp
> > > @@ -125,9 +125,13 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
> > >     /* Uniform block layout qualifiers get to overwrite each
> > >      * other (rightmost having priority), while all other
> > >      * qualifiers currently don't allow duplicates.
> > > +    *
> > > +    * Geometry shaders can have several layout qualifiers
> > > +    * assigning different stream ID values.
> > >      */
> > >  
> > > -   if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
> > > +   if ((state->stage != MESA_SHADER_GEOMETRY) &&
> > > +       (this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
> > >  				      ubo_layout_mask.flags.i |
> > >                                        ubo_binding_mask.flags.i)) != 0) {
> > 
> > I think this will allow multiply layout(location=) qualifiers on
> > geometry shader inputs / outputs.  So...
> > 
> > #version 150
> > #extension GL_ARB_separate_shader_objects: require
> > 
> > layout(location=1) layout(location=2) in vec4 v;
> > 
> > should generate an error, but I think this change will disable that error.
> > 
> 
> OK, we will test it and modify the patch accordingly.

The tests show that there is still an error, but a previous one.

With the original code the error messages are:

* Compiling shader 2: shaders/shader.geom
        Error: 0:8(1): error: duplicate layout(...) qualifiers
0:8(1): error: duplicate layout qualifiers used

While with the patch applied, the error message is:

* Compiling shader 2: shaders/shader.geom
        Error: 0:8(1): error: duplicate layout(...) qualifiers

If you prefer to have the "error: duplicate layout qualifiers used"
error message enabled, please say that, so I can write a patch for
it :-)

I made more tests just in case: if "v" is defined two times with
different location values, the error message is:

* Compiling shader 2: shaders/shader.geom
        Error: 0:10(1): error: geometry shader inputs must be arrays
0:11(1): error: geometry shader inputs must be arrays
0:11(28): error: `v' redeclared

Which reports the redeclaration of "v" input.

Thanks,

Sam