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

Submitted by Iago Toral Quiroga on June 18, 2014, 9:51 a.m.

Details

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

Not browsing as part of any series.

Commit Message

Iago Toral Quiroga June 18, 2014, 9:51 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       | 17 +++++++++++++++
 src/glsl/ast_type.cpp         | 39 +++++++++++++++++++++++++++++++++-
 src/glsl/glsl_parser.yy       | 49 +++++++++++++++++++++++++++++++++++++++++++
 src/glsl/glsl_parser_extras.h | 18 ++++++++++++++++
 src/glsl/glsl_types.h         |  5 +++++
 src/glsl/ir.h                 |  5 +++++
 7 files changed, 137 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index 56e7bd8..c8a3394 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 stream:1; /* Has stream value assigned  */
+         unsigned explicit_stream:1; /* stream value assigned explicitly 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 in GLSL 1.50 geometry shaders. */
+   unsigned stream;
+
    /** 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 132a955..c1bc0f9 100644
--- a/src/glsl/ast_to_hir.cpp
+++ b/src/glsl/ast_to_hir.cpp
@@ -2461,6 +2461,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.stream) {
+      var->data.stream = qual->stream;
+   }
+
    if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
       var->type = glsl_type::error_type;
       _mesa_glsl_error(loc, state,
@@ -5092,6 +5097,8 @@  ast_process_structure_or_interface_block(exec_list *instructions,
             interpret_interpolation_qualifier(qual, var_mode, state, &loc);
          fields[i].centroid = qual->flags.q.centroid ? 1 : 0;
          fields[i].sample = qual->flags.q.sample ? 1 : 0;
+         /* Only save explicitly defined streams in block's field */
+         fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
 
          if (qual->flags.q.row_major || qual->flags.q.column_major) {
             if (!qual->flags.q.uniform) {
@@ -5533,6 +5540,16 @@  ast_interface_block::hir(exec_list *instructions,
          var->data.sample = fields[i].sample;
          var->init_interface_type(block_type);
 
+         if (fields[i].stream != -1 &&
+             ((unsigned)fields[i].stream) != this->layout.stream) {
+            _mesa_glsl_error(&loc, state,
+                             "stream layout qualifier on "
+                             "interface block member `%s' does not match "
+                             "the interface block (%d and %d)",
+                             var->name, fields[i].stream, this->layout.stream);
+         }
+         var->data.stream = this->layout.stream;
+
          if (redeclaring_per_vertex) {
             ir_variable *earlier =
                get_variable_being_redeclared(var, loc, state,
diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
index 77053d5..daa3594 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 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,39 @@  ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
       this->max_vertices = q.max_vertices;
    }
 
+   if (state->stage == MESA_SHADER_GEOMETRY &&
+       state->has_explicit_attrib_stream()) {
+      if (q.flags.q.stream && q.stream >= state->ctx->Const.MaxVertexStreams) {
+         _mesa_glsl_error(loc, state,
+                          "`stream' value is bigger than MAX_VERTEX_STREAMS - 1 "
+                          "(%d > %d)",
+                          q.stream, state->ctx->Const.MaxVertexStreams - 1);
+      }
+      if (this->flags.q.explicit_stream &&
+          this->stream >= state->ctx->Const.MaxVertexStreams) {
+         _mesa_glsl_error(loc, state,
+                          "`stream' value is bigger than MAX_VERTEX_STREAMS - 1 "
+                          "(%d > %d)",
+                          this->stream, state->ctx->Const.MaxVertexStreams - 1);
+      }
+
+      if (!this->flags.q.explicit_stream) {
+         if (q.flags.q.stream) {
+            this->flags.q.stream = 1;
+            this->stream = q.stream;
+         } else if (!this->flags.q.stream && this->flags.q.out) {
+            /* Assign default global stream value */
+            this->flags.q.stream = 1;
+            this->stream = state->out_qualifier->stream;
+         }
+      } else {
+         if (q.flags.q.explicit_stream) {
+            _mesa_glsl_error(loc, state,
+                             "duplicate layout `stream' qualifier");
+         }
+      }
+   }
+
    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 97b6c3f..8509074 100644
--- a/src/glsl/glsl_parser.yy
+++ b/src/glsl/glsl_parser.yy
@@ -1395,6 +1395,22 @@  layout_qualifier_id:
          }
       }
 
+      if (state->stage == MESA_SHADER_GEOMETRY) {
+         if (match_layout_qualifier("stream", $1, state) == 0 &&
+             state->check_explicit_attrib_stream_allowed(& @3)) {
+            $$.flags.q.stream = 1;
+
+            if ($3 < 0) {
+               _mesa_glsl_error(& @3, state,
+                                "invalid stream %d specified", $3);
+               YYERROR;
+            } else {
+               $$.flags.q.explicit_stream = 1;
+               $$.stream = $3;
+            }
+         }
+      }
+
       static const char * const local_size_qualifiers[3] = {
          "local_size_x",
          "local_size_y",
@@ -1713,6 +1729,17 @@  storage_qualifier:
    {
       memset(& $$, 0, sizeof($$));
       $$.flags.q.out = 1;
+
+      if (state->stage == MESA_SHADER_GEOMETRY &&
+          state->has_explicit_attrib_stream()) {
+         /*
+          * According to the GL_ARB_gpu_shader5 spec, assign a default value
+          * to the stream
+          */
+          $$.flags.q.stream = 1;
+          $$.flags.q.explicit_stream = 0;
+          $$.stream = state->out_qualifier->stream;
+      }
    }
    | UNIFORM
    {
@@ -2381,6 +2408,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.stream && qualifier.stream != block->layout.stream) {
+               _mesa_glsl_error(& @1, state,
+                             "stream layout qualifier on "
+                             "interface block member does not match "
+                             "the interface block (%d and %d)",
+                             qualifier.stream, block->layout.stream);
+               YYERROR;
+         }
+      }
       $$ = block;
    }
    ;
@@ -2454,6 +2493,14 @@  basic_interface_block:
 
       block->layout.flags.i |= block_interface_qualifier;
 
+      if (state->stage == MESA_SHADER_GEOMETRY &&
+          state->has_explicit_attrib_stream()) {
+         /* Assign global layout's stream value. */
+         block->layout.flags.q.stream = 1;
+         block->layout.flags.q.explicit_stream = 0;
+         block->layout.stream = state->out_qualifier->stream;
+      }
+
       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 +2643,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_stream = 0;
       }
       $$ = NULL;
    }
diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
index a59090f..088a9f3 100644
--- a/src/glsl/glsl_parser_extras.h
+++ b/src/glsl/glsl_parser_extras.h
@@ -119,6 +119,19 @@  struct _mesa_glsl_parse_state {
       return check_version(130, 300, locp, "bit-wise operations are forbidden");
    }
 
+   bool check_explicit_attrib_stream_allowed(YYLTYPE *locp)
+   {
+      if (!this->has_explicit_attrib_stream()) {
+         const char *const requirement = "GL_ARB_gpu_shader5 extension or GLSL 400";
+
+         _mesa_glsl_error(locp, this, "explicit stream requires %s",
+                          requirement);
+         return false;
+      }
+
+      return true;
+   }
+
    bool check_explicit_attrib_location_allowed(YYLTYPE *locp,
                                                const ir_variable *var)
    {
@@ -166,6 +179,11 @@  struct _mesa_glsl_parse_state {
       return true;
    }
 
+   bool has_explicit_attrib_stream() const
+   {
+      return ARB_gpu_shader5_enable || is_version(400, 0);
+   }
+
    bool has_explicit_attrib_location() const
    {
       return ARB_explicit_attrib_location_enable || is_version(330, 300);
diff --git a/src/glsl/glsl_types.h b/src/glsl/glsl_types.h
index f6d4a02..007ff05 100644
--- a/src/glsl/glsl_types.h
+++ b/src/glsl/glsl_types.h
@@ -671,6 +671,11 @@  struct glsl_struct_field {
     * in ir_variable::sample). 0 otherwise.
     */
    unsigned sample:1;
+   /**
+    * For interface blocks, it has a value if this variable uses multiple vertex
+    * streams (as in ir_variable::stream). -1 otherwise.
+    */
+   int stream;
 };
 
 static inline unsigned int
diff --git a/src/glsl/ir.h b/src/glsl/ir.h
index b4e52d3..dbbabb5 100644
--- a/src/glsl/ir.h
+++ b/src/glsl/ir.h
@@ -705,6 +705,11 @@  public:
        */
       int location;
 
+      /*
+       * Vertex stream output identifier.
+       */
+      unsigned stream;
+
       /**
        * output index for dual source blending.
        */

Comments

On 06/18/2014 02:51 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>

A few minor nits below.  With those fixed, this patch is

Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

> ---
>  src/glsl/ast.h                |  5 +++++
>  src/glsl/ast_to_hir.cpp       | 17 +++++++++++++++
>  src/glsl/ast_type.cpp         | 39 +++++++++++++++++++++++++++++++++-
>  src/glsl/glsl_parser.yy       | 49 +++++++++++++++++++++++++++++++++++++++++++
>  src/glsl/glsl_parser_extras.h | 18 ++++++++++++++++
>  src/glsl/glsl_types.h         |  5 +++++
>  src/glsl/ir.h                 |  5 +++++
>  7 files changed, 137 insertions(+), 1 deletion(-)
> 
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 56e7bd8..c8a3394 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 stream:1; /* Has stream value assigned  */
> +         unsigned explicit_stream:1; /* stream value assigned explicitly by shader code */

End-of-line comments should begin with /**< for Doxygen.

>           /** \} */
>        }
>        /** \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 in GLSL 1.50 geometry shaders. */
> +   unsigned stream;
> +
>     /** 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 132a955..c1bc0f9 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -2461,6 +2461,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.stream) {
> +      var->data.stream = qual->stream;
> +   }
> +
>     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
>        var->type = glsl_type::error_type;
>        _mesa_glsl_error(loc, state,
> @@ -5092,6 +5097,8 @@ ast_process_structure_or_interface_block(exec_list *instructions,
>              interpret_interpolation_qualifier(qual, var_mode, state, &loc);
>           fields[i].centroid = qual->flags.q.centroid ? 1 : 0;
>           fields[i].sample = qual->flags.q.sample ? 1 : 0;

Add a blank link here.

> +         /* Only save explicitly defined streams in block's field */

And put the */ on it's own line.

> +         fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
>  
>           if (qual->flags.q.row_major || qual->flags.q.column_major) {
>              if (!qual->flags.q.uniform) {
> @@ -5533,6 +5540,16 @@ ast_interface_block::hir(exec_list *instructions,
>           var->data.sample = fields[i].sample;
>           var->init_interface_type(block_type);
>  
> +         if (fields[i].stream != -1 &&
> +             ((unsigned)fields[i].stream) != this->layout.stream) {
> +            _mesa_glsl_error(&loc, state,
> +                             "stream layout qualifier on "
> +                             "interface block member `%s' does not match "
> +                             "the interface block (%d and %d)",

In other places we generally say "%d vs %d".

> +                             var->name, fields[i].stream, this->layout.stream);
> +         }

Blank line here.

> +         var->data.stream = this->layout.stream;
> +
>           if (redeclaring_per_vertex) {
>              ir_variable *earlier =
>                 get_variable_being_redeclared(var, loc, state,
> diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
> index 77053d5..daa3594 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 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,39 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>        this->max_vertices = q.max_vertices;
>     }
>  
> +   if (state->stage == MESA_SHADER_GEOMETRY &&
> +       state->has_explicit_attrib_stream()) {
> +      if (q.flags.q.stream && q.stream >= state->ctx->Const.MaxVertexStreams) {
> +         _mesa_glsl_error(loc, state,
> +                          "`stream' value is bigger than MAX_VERTEX_STREAMS - 1 "

Everywhere else we use "larger" (instead of "bigger").

> +                          "(%d > %d)",
> +                          q.stream, state->ctx->Const.MaxVertexStreams - 1);
> +      }
> +      if (this->flags.q.explicit_stream &&
> +          this->stream >= state->ctx->Const.MaxVertexStreams) {
> +         _mesa_glsl_error(loc, state,
> +                          "`stream' value is bigger than MAX_VERTEX_STREAMS - 1 "

Same here.

> +                          "(%d > %d)",
> +                          this->stream, state->ctx->Const.MaxVertexStreams - 1);
> +      }
> +
> +      if (!this->flags.q.explicit_stream) {
> +         if (q.flags.q.stream) {
> +            this->flags.q.stream = 1;
> +            this->stream = q.stream;
> +         } else if (!this->flags.q.stream && this->flags.q.out) {
> +            /* Assign default global stream value */
> +            this->flags.q.stream = 1;
> +            this->stream = state->out_qualifier->stream;
> +         }
> +      } else {
> +         if (q.flags.q.explicit_stream) {
> +            _mesa_glsl_error(loc, state,
> +                             "duplicate layout `stream' qualifier");
> +         }
> +      }
> +   }
> +
>     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 97b6c3f..8509074 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -1395,6 +1395,22 @@ layout_qualifier_id:
>           }
>        }
>  
> +      if (state->stage == MESA_SHADER_GEOMETRY) {
> +         if (match_layout_qualifier("stream", $1, state) == 0 &&
> +             state->check_explicit_attrib_stream_allowed(& @3)) {
> +            $$.flags.q.stream = 1;
> +
> +            if ($3 < 0) {
> +               _mesa_glsl_error(& @3, state,
> +                                "invalid stream %d specified", $3);
> +               YYERROR;
> +            } else {
> +               $$.flags.q.explicit_stream = 1;
> +               $$.stream = $3;
> +            }
> +         }
> +      }
> +
>        static const char * const local_size_qualifiers[3] = {
>           "local_size_x",
>           "local_size_y",
> @@ -1713,6 +1729,17 @@ storage_qualifier:
>     {
>        memset(& $$, 0, sizeof($$));
>        $$.flags.q.out = 1;
> +
> +      if (state->stage == MESA_SHADER_GEOMETRY &&
> +          state->has_explicit_attrib_stream()) {
> +         /*
> +          * According to the GL_ARB_gpu_shader5 spec, assign a default value
> +          * to the stream
> +          */

Please replace this comment with the following spec quotation:

         /* Section 4.3.8.2 (Output Layout Qualifiers) of the OpenGL 4.00
          * spec says:
          *
          *     "If the block or variable is declared with the stream
          *     identifier, it is associated with the specified stream;
          *     otherwise, it is associated with the current default stream."
          */

> +          $$.flags.q.stream = 1;
> +          $$.flags.q.explicit_stream = 0;
> +          $$.stream = state->out_qualifier->stream;
> +      }
>     }
>     | UNIFORM
>     {
> @@ -2381,6 +2408,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.stream && qualifier.stream != block->layout.stream) {
> +               _mesa_glsl_error(& @1, state,
> +                             "stream layout qualifier on "
> +                             "interface block member does not match "
> +                             "the interface block (%d and %d)",

vs

> +                             qualifier.stream, block->layout.stream);
> +               YYERROR;
> +         }
> +      }
>        $$ = block;
>     }
>     ;
> @@ -2454,6 +2493,14 @@ basic_interface_block:
>  
>        block->layout.flags.i |= block_interface_qualifier;
>  
> +      if (state->stage == MESA_SHADER_GEOMETRY &&
> +          state->has_explicit_attrib_stream()) {
> +         /* Assign global layout's stream value. */
> +         block->layout.flags.q.stream = 1;
> +         block->layout.flags.q.explicit_stream = 0;
> +         block->layout.stream = state->out_qualifier->stream;
> +      }
> +
>        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 +2643,8 @@ layout_defaults:
>           }
>           if (!state->out_qualifier->merge_qualifier(& @1, state, $1))
>              YYERROR;

Blank line here.

> +         /* Allow future assigments of global out's stream id value */
> +         state->out_qualifier->flags.q.explicit_stream = 0;
>        }
>        $$ = NULL;
>     }
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index a59090f..088a9f3 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -119,6 +119,19 @@ struct _mesa_glsl_parse_state {
>        return check_version(130, 300, locp, "bit-wise operations are forbidden");
>     }
>  
> +   bool check_explicit_attrib_stream_allowed(YYLTYPE *locp)
> +   {
> +      if (!this->has_explicit_attrib_stream()) {
> +         const char *const requirement = "GL_ARB_gpu_shader5 extension or GLSL 400";
> +
> +         _mesa_glsl_error(locp, this, "explicit stream requires %s",
> +                          requirement);
> +         return false;
> +      }
> +
> +      return true;
> +   }
> +
>     bool check_explicit_attrib_location_allowed(YYLTYPE *locp,
>                                                 const ir_variable *var)
>     {
> @@ -166,6 +179,11 @@ struct _mesa_glsl_parse_state {
>        return true;
>     }
>  
> +   bool has_explicit_attrib_stream() const
> +   {
> +      return ARB_gpu_shader5_enable || is_version(400, 0);
> +   }
> +
>     bool has_explicit_attrib_location() const
>     {
>        return ARB_explicit_attrib_location_enable || is_version(330, 300);
> diff --git a/src/glsl/glsl_types.h b/src/glsl/glsl_types.h
> index f6d4a02..007ff05 100644
> --- a/src/glsl/glsl_types.h
> +++ b/src/glsl/glsl_types.h
> @@ -671,6 +671,11 @@ struct glsl_struct_field {
>      * in ir_variable::sample). 0 otherwise.
>      */
>     unsigned sample:1;

Blank line here.

> +   /**
> +    * For interface blocks, it has a value if this variable uses multiple vertex
> +    * streams (as in ir_variable::stream). -1 otherwise.
> +    */
> +   int stream;
>  };
>  
>  static inline unsigned int
> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
> index b4e52d3..dbbabb5 100644
> --- a/src/glsl/ir.h
> +++ b/src/glsl/ir.h
> @@ -705,6 +705,11 @@ public:
>         */
>        int location;
>  
> +      /*

         /**

> +       * Vertex stream output identifier.
> +       */
> +      unsigned stream;
> +
>        /**
>         * output index for dual source blending.
>         */
>
On Wednesday, June 18, 2014 11:16:47 AM Ian Romanick wrote:
> On 06/18/2014 02:51 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>
> 
> A few minor nits below.  With those fixed, this patch is
> 
> Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
> 
> > ---
> >  src/glsl/ast.h                |  5 +++++
> >  src/glsl/ast_to_hir.cpp       | 17 +++++++++++++++
> >  src/glsl/ast_type.cpp         | 39 +++++++++++++++++++++++++++++++++-
> >  src/glsl/glsl_parser.yy       | 49 
+++++++++++++++++++++++++++++++++++++++++++
> >  src/glsl/glsl_parser_extras.h | 18 ++++++++++++++++
> >  src/glsl/glsl_types.h         |  5 +++++
> >  src/glsl/ir.h                 |  5 +++++
> >  7 files changed, 137 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> > index 56e7bd8..c8a3394 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 stream:1; /* Has stream value assigned  */
> > +         unsigned explicit_stream:1; /* stream value assigned explicitly 
by shader code */
> 
> End-of-line comments should begin with /**< for Doxygen.
> 
> >           /** \} */
> >        }
> >        /** \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 in GLSL 1.50 geometry shaders. */
> > +   unsigned stream;
> > +
> >     /** 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 132a955..c1bc0f9 100644
> > --- a/src/glsl/ast_to_hir.cpp
> > +++ b/src/glsl/ast_to_hir.cpp
> > @@ -2461,6 +2461,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.stream) {
> > +      var->data.stream = qual->stream;
> > +   }
> > +
> >     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
> >        var->type = glsl_type::error_type;
> >        _mesa_glsl_error(loc, state,
> > @@ -5092,6 +5097,8 @@ ast_process_structure_or_interface_block(exec_list 
*instructions,
> >              interpret_interpolation_qualifier(qual, var_mode, state, 
&loc);
> >           fields[i].centroid = qual->flags.q.centroid ? 1 : 0;
> >           fields[i].sample = qual->flags.q.sample ? 1 : 0;
> 
> Add a blank link here.
> 
> > +         /* Only save explicitly defined streams in block's field */
> 
> And put the */ on it's own line.

Ian, I think you are one of the few people that do that.  Mesa overwhelmingly 
starts and ends comments on the same line, where they fit on one line:

(attempt at finding /* on one line and */ on the next line):
$ git grep -A1 '/\*' | grep '\*/' | grep -v '/\*.*\*/' | wc -l
2354

(attempt at finding one line /* ... */ comments):
$ git grep '/\*.*\*/' | wc -l
35025

So I would leave it as is.  But it's not a big deal...

--Ken
On Wed, 2014-06-18 at 11:16 -0700, Ian Romanick wrote:
> On 06/18/2014 02:51 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>
> 
> A few minor nits below.  With those fixed, this patch is
> 
> Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>

Thanks for your review! I will work on it.

Sam
On Wed, 2014-06-18 at 11:16 -0700, Ian Romanick wrote:
(...)
> 
> Please replace this comment with the following spec quotation:
> 
>          /* Section 4.3.8.2 (Output Layout Qualifiers) of the OpenGL 4.00

I think you meant the GLSL spec.

>           * spec says:
>           *
>           *     "If the block or variable is declared with the stream
>           *     identifier, it is associated with the specified stream;
>           *     otherwise, it is associated with the current default stream."
>           */
> 
> > +          $$.flags.q.stream = 1;
> > +          $$.flags.q.explicit_stream = 0;
> > +          $$.stream = state->out_qualifier->stream;
> > +      }

Iago
On 06/18/2014 01:45 PM, Kenneth Graunke wrote:
> On Wednesday, June 18, 2014 11:16:47 AM Ian Romanick wrote:
>> On 06/18/2014 02:51 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>
>>
>> A few minor nits below.  With those fixed, this patch is
>>
>> Reviewed-by: Ian Romanick <ian.d.romanick@intel.com>
>>
>>> ---
>>>  src/glsl/ast.h                |  5 +++++
>>>  src/glsl/ast_to_hir.cpp       | 17 +++++++++++++++
>>>  src/glsl/ast_type.cpp         | 39 +++++++++++++++++++++++++++++++++-
>>>  src/glsl/glsl_parser.yy       | 49 
> +++++++++++++++++++++++++++++++++++++++++++
>>>  src/glsl/glsl_parser_extras.h | 18 ++++++++++++++++
>>>  src/glsl/glsl_types.h         |  5 +++++
>>>  src/glsl/ir.h                 |  5 +++++
>>>  7 files changed, 137 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
>>> index 56e7bd8..c8a3394 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 stream:1; /* Has stream value assigned  */
>>> +         unsigned explicit_stream:1; /* stream value assigned explicitly 
> by shader code */
>>
>> End-of-line comments should begin with /**< for Doxygen.
>>
>>>           /** \} */
>>>        }
>>>        /** \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 in GLSL 1.50 geometry shaders. */
>>> +   unsigned stream;
>>> +
>>>     /** 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 132a955..c1bc0f9 100644
>>> --- a/src/glsl/ast_to_hir.cpp
>>> +++ b/src/glsl/ast_to_hir.cpp
>>> @@ -2461,6 +2461,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.stream) {
>>> +      var->data.stream = qual->stream;
>>> +   }
>>> +
>>>     if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
>>>        var->type = glsl_type::error_type;
>>>        _mesa_glsl_error(loc, state,
>>> @@ -5092,6 +5097,8 @@ ast_process_structure_or_interface_block(exec_list 
> *instructions,
>>>              interpret_interpolation_qualifier(qual, var_mode, state, 
> &loc);
>>>           fields[i].centroid = qual->flags.q.centroid ? 1 : 0;
>>>           fields[i].sample = qual->flags.q.sample ? 1 : 0;
>>
>> Add a blank link here.
>>
>>> +         /* Only save explicitly defined streams in block's field */
>>
>> And put the */ on it's own line.
> 
> Ian, I think you are one of the few people that do that.  Mesa overwhelmingly 
> starts and ends comments on the same line, where they fit on one line:
> 
> (attempt at finding /* on one line and */ on the next line):
> $ git grep -A1 '/\*' | grep '\*/' | grep -v '/\*.*\*/' | wc -l
> 2354
> 
> (attempt at finding one line /* ... */ comments):
> $ git grep '/\*.*\*/' | wc -l
> 35025

I think the expressions you actually wanted are below.  Both cases
ignore everything in include/, src/gallium/, and src/gtest/ because I
don't think the code style of those is relevant for core Mesa.  An
argument could be made that src/drivers/dri should also be excluded
(src/drivers/dri/nouveau uses hard-tabs for indentionat, etc.).  The
first expression tries to capture only /* ... */ comments that are
alone on a line. End-of-line comments or comments embedded in the
middle of code are excluded.

$ git grep '^[[:space:]]*/\*[^@*].*\*/$' src/mesa/ src/glsl/ | grep -v ';' | wc -l
7614

$ git grep -A1 '/\*' src/glsl/ src/mesa/ | grep '\*/' | grep -v '/\*.*\*/' | wc -l
1205

6:1 is a lot different ratio than 17:1.

I care a lot less about "what is often done" than I do about "what
should be done".  If there is an argument to be made that stand-alone
comments (not on a line with other code) are better, that would be good
data to have.  Classically, Mesa has opted for more whitespace because
it makes the code easier for humans to parse.  This is part of the
reason we don't use //-style comments even in C++ code.

I would much rather read

        fields[i].sample = qual->flags.q.sample ? 1 : 0;

        /* Only save explicitly defined streams in block's field.
         */
        fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;

than

        fields[i].sample = qual->flags.q.sample ? 1 : 0;
        /* Only save explicitly defined streams in block's field */
        fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;

I'd also like to hear Brian's opinion, since most of Mesa style is his
doing.

> So I would leave it as is.  But it's not a big deal...
> 
> --Ken
On Fri, 2014-06-20 at 10:46 -0700, Ian Romanick wrote:

> I care a lot less about "what is often done" than I do about "what
> should be done".  If there is an argument to be made that stand-alone
> comments (not on a line with other code) are better, that would be good
> data to have. 

Well I'm not sure how common a use case this is for Mesa devs but when
doing any coding on a laptop the less wasted lines the better.
Especially when the aspect ratio of all new laptops is suited to
watching dvds rather than any kind of document viewing/editing.
I personally do most of my Mesa contributions on the train travelling to
and from work.
For the same reason I've always wondered why the preferred width of
lines is 78 columns? Is this just a historical thing? Or does it make
sense to use this restriction e.g for debugging in the terminal? The
devinfo.html doesn't give an actual reason, and 78 columns seems rather
small considering the size and resolutions of modern screens.

>  Classically, Mesa has opted for more whitespace because
> it makes the code easier for humans to parse.  This is part of the
> reason we don't use //-style comments even in C++ code.
> 
> I would much rather read
> 
>         fields[i].sample = qual->flags.q.sample ? 1 : 0;
> 
>         /* Only save explicitly defined streams in block's field.
>          */
>         fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
> 
> than
> 
>         fields[i].sample = qual->flags.q.sample ? 1 : 0;
>         /* Only save explicitly defined streams in block's field */
>         fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
> 
> I'd also like to hear Brian's opinion, since most of Mesa style is his
> doing.
On Fri, Jun 20, 2014 at 11:17 PM, Timothy Arceri <t_arceri@yahoo.com.au> wrote:
> On Fri, 2014-06-20 at 10:46 -0700, Ian Romanick wrote:
>
>> I care a lot less about "what is often done" than I do about "what
>> should be done".  If there is an argument to be made that stand-alone
>> comments (not on a line with other code) are better, that would be good
>> data to have.
>
> Well I'm not sure how common a use case this is for Mesa devs but when
> doing any coding on a laptop the less wasted lines the better.
> Especially when the aspect ratio of all new laptops is suited to
> watching dvds rather than any kind of document viewing/editing.
> I personally do most of my Mesa contributions on the train travelling to
> and from work.
> For the same reason I've always wondered why the preferred width of
> lines is 78 columns? Is this just a historical thing? Or does it make
> sense to use this restriction e.g for debugging in the terminal? The
> devinfo.html doesn't give an actual reason, and 78 columns seems rather
> small considering the size and resolutions of modern screens.

I think a shorter line limit is *especially* important in the age of
modern screens that are way too wide. If you were to, say, double the
line length to 160, some lines would be very long but most would
remain short. So you would lose code density (half the window would be
empty), and you would lose the ability to have more windows
side-by-side. Also, it so happens that 2 80-char windows fit
*perfectly* side-by-side on a rotated 24" monitor (1200x1920).

  -ilia
On 06/20/2014 08:17 PM, Timothy Arceri wrote:
> On Fri, 2014-06-20 at 10:46 -0700, Ian Romanick wrote:
> 
>> I care a lot less about "what is often done" than I do about "what
>> should be done".  If there is an argument to be made that stand-alone
>> comments (not on a line with other code) are better, that would be good
>> data to have. 
> 
> Well I'm not sure how common a use case this is for Mesa devs but when
> doing any coding on a laptop the less wasted lines the better.
> Especially when the aspect ratio of all new laptops is suited to
> watching dvds rather than any kind of document viewing/editing.
> I personally do most of my Mesa contributions on the train travelling to
> and from work.
> For the same reason I've always wondered why the preferred width of
> lines is 78 columns? Is this just a historical thing? Or does it make
> sense to use this restriction e.g for debugging in the terminal? The
> devinfo.html doesn't give an actual reason, and 78 columns seems rather
> small considering the size and resolutions of modern screens.

There have been numerous usability studies that show rather conclusively
that it is much easier for humans to read narrower lines of text.  This
is part of the reason newspaper and magazines format text in multiple
columns per page rather than fitting to page width.  For example:

Dyson, M. C. (2004) How physical text layout affects reading from
screen. Behaviour and Information Technology, 23 (6). pp. 377-393. ISSN
1362-3001.

>>  Classically, Mesa has opted for more whitespace because
>> it makes the code easier for humans to parse.  This is part of the
>> reason we don't use //-style comments even in C++ code.
>>
>> I would much rather read
>>
>>         fields[i].sample = qual->flags.q.sample ? 1 : 0;
>>
>>         /* Only save explicitly defined streams in block's field.
>>          */
>>         fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
>>
>> than
>>
>>         fields[i].sample = qual->flags.q.sample ? 1 : 0;
>>         /* Only save explicitly defined streams in block's field */
>>         fields[i].stream = qual->flags.q.explicit_stream ? qual->stream : -1;
>>
>> I'd also like to hear Brian's opinion, since most of Mesa style is his
>> doing.
>