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

[Kenneth Graunke]

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 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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140618/56ee7e2e/attachment.sig>