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

Ian Romanick idr at freedesktop.org
Wed Jun 18 11:16:47 PDT 2014


On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote:
> From: Samuel Iglesias Gonsalvez <siglesias at 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 at igalia.com>

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

Reviewed-by: Ian Romanick <ian.d.romanick at 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.
>         */
> 



More information about the mesa-dev mailing list