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

[Ian Romanick]

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140620/0e187d38/attachment.sig>