On 06/18/2014 02:51 AM, Iago Toral Quiroga wrote: > From: Samuel Iglesias Gonsalvez <sigles...@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 <sigles...@igalia.com>
A few minor nits below. With those fixed, this patch is Reviewed-by: Ian Romanick <ian.d.roman...@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. > */ > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev