On 06/11/2014 01:40 AM, Chris Forbes wrote:
> On Wed, Jun 11, 2014 at 7:49 PM, Iago Toral Quiroga <ito...@igalia.com> wrote:
>> @@ -509,6 +509,8 @@ struct ast_type_qualifier {
>> /** \name Layout qualifiers for GL_ARB_gpu_shader5 */
>> /** \{ */
>> unsigned invocations:1;
>> + unsigned streamId:1; /* Has streamId value assigned */
>> + unsigned explicit_streamId:1; /* streamId value assigned
>> explicitely by shader code */
>
> s/explicitely/explicitly/
>
> I'd call these stream / explicit_stream (or if you must, stream_id /
> explicit_stream_id). None of the other members here are using camel
> case, and explicit_streamId is a crazy mix.
I agree.
>> /** \} */
>> }
>> /** \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 ID in GLSL 1.50 geometry shaders. */
>> + unsigned streamId;
>> +
>> /** 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 140bb74..ab0f50f 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -2426,6 +2426,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.streamId) {
>> + var->data.streamId = qual->streamId;
>> + }
>> +
>> if (qual->flags.q.attribute && state->stage != MESA_SHADER_VERTEX) {
>> var->type = glsl_type::error_type;
>> _mesa_glsl_error(loc, state,
>> @@ -5455,6 +5460,7 @@ ast_interface_block::hir(exec_list *instructions,
>> var->data.centroid = fields[i].centroid;
>> var->data.sample = fields[i].sample;
>> var->init_interface_type(block_type);
>> + var->data.streamId = this->layout.streamId;
>>
>> if (redeclaring_per_vertex) {
>> ir_variable *earlier =
>> diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
>> index 0ee2c49..749f161 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 ID 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,19 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>> this->max_vertices = q.max_vertices;
>> }
>>
>> + if (state->stage == MESA_SHADER_GEOMETRY &&
>> + state->ARB_gpu_shader5_enable &&
>> + !this->flags.q.explicit_streamId) {
>> + if (q.flags.q.streamId) {
>> + this->flags.q.streamId = 1;
>> + this->streamId = q.streamId;
>> + } else if(this->flags.q.streamId == 0) {
>
> !this->flags.q.streamId, for consistency with the explicit_streamId check
> above.
>
>> + /* Assign default global streamId value */
>> + this->flags.q.streamId = 1;
>> + this->streamId = state->out_qualifier->streamId;
>> + }
>> + }
>> +
>> 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 eddab05..caebfe7 100644
>> --- a/src/glsl/glsl_parser.yy
>> +++ b/src/glsl/glsl_parser.yy
>> @@ -1395,6 +1395,21 @@ layout_qualifier_id:
>> }
>> }
>>
>> + if (state->ARB_gpu_shader5_enable && state->stage ==
>> MESA_SHADER_GEOMETRY) {
>
> This check (and others like it) should really also pass if
> state->is_version(400, 0) as well, so we
> don't have to come back and change them all again.
In similar situations we add a method like check_explicit_stream_allowed
that encapsulates the various checks. See
_mesa_glsl_parser_state::check_explicit_attrib_location_allowed (in
glsl_parser_extras.h).
>> + if (match_layout_qualifier("stream", $1, state) == 0) {
>> + $$.flags.q.streamId = 1;
>> +
>> + if (($3 < 0) || ($3 >= MAX_VERTEX_STREAMS)) {
>
> This should really be checking against the real limit... but I see
> there's a precedent for this in the
> equivalent MAX_GEOMETRY_SHADER_INVOCATIONS case, which is much more
> likely to vary
> across hardware...
If there is a hard, upper limit, we may check against that in the
parser. Sometimes we store the value in a small bitfield, so checking
against the hard limit prevents trying to store 0xDEADBEEF in 3 bits or
something. :)
>> + _mesa_glsl_error(& @3, state,
>> + "invalid stream %d specified", $3);
>> + YYERROR;
>> + } else {
>> + $$.flags.q.explicit_streamId = 1;
>> + $$.streamId = $3;
>> + }
>> + }
>> + }
>> +
>
>> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>> index b4e52d3..8cc58af 100644
>> --- a/src/glsl/ir.h
>> +++ b/src/glsl/ir.h
>> @@ -705,6 +705,11 @@ public:
>> */
>> int location;
>>
>> + /*
>> + * Vertex stream output identifier.
>> + */
>> + unsigned streamId;
>
> Same comment as in the ast about naming.
>
>> +
>> /**
>> * output index for dual source blending.
>> */
>> --
>> 1.9.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
