[Mesa-dev] [PATCH v2 03/14] glsl: Implement parser support for atomic counters.
Francisco Jerez
currojerez at riseup.net
Wed Oct 23 12:38:41 PDT 2013
Thank you for your comments,
I've rebased my atomic counters branch on top of master [1] and I think
I've taken into account everything you had pointed out.
The rebase has turned out to be especially painful this time. To avoid
wasting more time rebasing patches that no-one is going to have a look
at, I'm going to start merging the ones that have already got any review
by the end of this week.
Thank you.
[1] http://cgit.freedesktop.org/~currojerez/mesa/log/?h=atomic-counters
Ian Romanick <idr at freedesktop.org> writes:
> On 10/01/2013 07:15 PM, Francisco Jerez wrote:
>> v2: Mark atomic counters as read-only variables. Move offset overlap
>> code to the linker. Use the contains_atomic() convenience method.
>> ---
>> src/glsl/ast.h | 15 ++++++++++++
>> src/glsl/ast_to_hir.cpp | 57 ++++++++++++++++++++++++++++++++++++++++++-
>> src/glsl/ast_type.cpp | 13 ++++++++--
>> src/glsl/glsl_lexer.ll | 2 +-
>> src/glsl/glsl_parser.yy | 13 ++++++++--
>> src/glsl/glsl_parser_extras.h | 4 +++
>> 6 files changed, 98 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
>> index 97905c6..5c214b6 100644
>> --- a/src/glsl/ast.h
>> +++ b/src/glsl/ast.h
>> @@ -385,6 +385,12 @@ struct ast_type_qualifier {
>> */
>> unsigned explicit_binding:1;
>>
>> + /**
>> + * Flag set if GL_ARB_shader_atomic counter "offset" layout
>> + * qualifier is used.
>> + */
>> + unsigned explicit_offset:1;
>> +
>> /** \name Layout qualifiers for GL_AMD_conservative_depth */
>> /** \{ */
>> unsigned depth_any:1;
>> @@ -448,6 +454,15 @@ struct ast_type_qualifier {
>> int binding;
>>
>> /**
>> + * Offset specified via GL_ARB_shader_atomic_counter's "offset"
>> + * keyword.
>> + *
>> + * \note
>> + * This field is only valid if \c explicit_offset is set.
>> + */
>> + int offset;
>> +
>> + /**
>> * Return true if and only if an interpolation qualifier is present.
>> */
>> bool has_interpolation() const;
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index db59d0a..b373611 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -1956,10 +1956,19 @@ validate_binding_qualifier(struct _mesa_glsl_parse_state *state,
>>
>> return false;
>> }
>> + } else if (var->type->contains_atomic()) {
>> + if (unsigned(qual->binding) >= ctx->Const.MaxAtomicBufferBindings) {
>> + _mesa_glsl_error(loc, state, "layout(binding = %d) exceeds the "
>> + " maximum number of atomic counter buffer bindings"
>> + "(%d)", qual->binding,
>> + ctx->Const.MaxAtomicBufferBindings);
>> +
>> + return false;
>> + }
>> } else {
>> _mesa_glsl_error(loc, state,
>> "the \"binding\" qualifier only applies to uniform "
>> - "blocks, samplers, or arrays of samplers");
>> + "blocks, samplers, atomic counters, or arrays thereof");
>> return false;
>> }
>>
>> @@ -2229,6 +2238,29 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,
>> var->binding = qual->binding;
>> }
>>
>> + if (var->type->contains_atomic()) {
>> + if (var->mode == ir_var_uniform) {
>> + if (var->explicit_binding) {
>> + unsigned &offset = state->atomic_counter_offsets[var->binding];
>
> I had always been taught that references should almost always be const
> because...
>
>> +
>> + if (offset % ATOMIC_COUNTER_SIZE)
>> + _mesa_glsl_error(loc, state,
>> + "misaligned atomic counter offset");
>> +
>> + var->atomic.offset = offset;
>> + offset += var->type->atomic_size();
>
> ...it is completely non-obvious that this line of code modifies some
> other storage. You can kind of get away with it here because the
> declaration and the use are so close, but I'd prefer it be changed to a
> pointer. That keeps the shortened notation, but it also makes it
> obvious that other storage is being modified:
>
> *offset += var->type->atomic_size();
>
>> +
>> + } else {
>> + _mesa_glsl_error(loc, state,
>> + "atomic counters require explicit binding point");
>> + }
>> + } else if (var->mode != ir_var_function_in) {
>> + _mesa_glsl_error(loc, state, "atomic counters may only be declared as "
>> + "function parameters or uniform-qualified "
>> + "global variables");
>> + }
>> + }
>> +
>> /* Does the declaration use the deprecated 'attribute' or 'varying'
>> * keywords?
>> */
>> @@ -2729,6 +2761,18 @@ ast_declarator_list::hir(exec_list *instructions,
>> (void) this->type->specifier->hir(instructions, state);
>>
>> decl_type = this->type->glsl_type(& type_name, state);
>> +
>> + /* An offset-qualified atomic counter declaration sets the default
>> + * offset for the next declaration within the same atomic counter
>> + * buffer.
>> + */
>> + if (decl_type && decl_type->contains_atomic()) {
>> + if (type->qualifier.flags.q.explicit_binding &&
>> + type->qualifier.flags.q.explicit_offset)
>> + state->atomic_counter_offsets[type->qualifier.binding] =
>> + type->qualifier.offset;
>> + }
>> +
>> if (this->declarations.is_empty()) {
>> /* If there is no structure involved in the program text, there are two
>> * possible scenarios:
>> @@ -2758,6 +2802,11 @@ ast_declarator_list::hir(exec_list *instructions,
>> _mesa_glsl_error(&loc, state,
>> "invalid type `%s' in empty declaration",
>> type_name);
>> + } else if (decl_type->base_type == GLSL_TYPE_ATOMIC_UINT) {
>> + /* Empty atomic counter declarations are allowed and useful
>> + * to set the default offset qualifier.
>> + */
>> + return NULL;
>> } else if (this->type->qualifier.precision != ast_precision_none) {
>> if (this->type->specifier->structure != NULL) {
>> _mesa_glsl_error(&loc, state,
>> @@ -4479,6 +4528,12 @@ ast_process_structure_or_interface_block(exec_list *instructions,
>> "uniform in non-default uniform block contains sampler");
>> }
>>
>
> Could you add a comment here:
>
> /* FINISHME: Add a spec quotation here once updated spec langauge
> * FINISHME: is available.
> */
>> + if (field_type->contains_atomic()) {
>> + YYLTYPE loc = decl_list->get_location();
>> + _mesa_glsl_error(&loc, state, "atomic counter in structure or "
>> + "uniform block");
>> + }
>> +
>> const struct ast_type_qualifier *const qual =
>> & decl_list->type->qualifier;
>> if (qual->flags.q.std140 ||
>> diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp
>> index 8aabd95..2b088bf 100644
>> --- a/src/glsl/ast_type.cpp
>> +++ b/src/glsl/ast_type.cpp
>> @@ -72,7 +72,8 @@ ast_type_qualifier::has_layout() const
>> || this->flags.q.packed
>> || this->flags.q.explicit_location
>> || this->flags.q.explicit_index
>> - || this->flags.q.explicit_binding;
>> + || this->flags.q.explicit_binding
>> + || this->flags.q.explicit_offset;
>> }
>>
>> bool
>> @@ -121,13 +122,18 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>> ubo_layout_mask.flags.q.packed = 1;
>> ubo_layout_mask.flags.q.shared = 1;
>>
>> + ast_type_qualifier ubo_binding_mask;
>> + ubo_binding_mask.flags.q.explicit_binding = 1;
>> + ubo_binding_mask.flags.q.explicit_offset = 1;
>> +
>> /* Uniform block layout qualifiers get to overwrite each
>> * other (rightmost having priority), while all other
>> * qualifiers currently don't allow duplicates.
>> */
>>
>> if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |
>> - ubo_layout_mask.flags.i)) != 0) {
>> + ubo_layout_mask.flags.i |
>> + ubo_binding_mask.flags.i)) != 0) {
>> _mesa_glsl_error(loc, state,
>> "duplicate layout qualifiers used");
>> return false;
>> @@ -168,6 +174,9 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,
>> if (q.flags.q.explicit_binding)
>> this->binding = q.binding;
>>
>> + if (q.flags.q.explicit_offset)
>> + this->offset = q.offset;
>> +
>> if (q.precision != ast_precision_none)
>> this->precision = q.precision;
>>
>> diff --git a/src/glsl/glsl_lexer.ll b/src/glsl/glsl_lexer.ll
>> index e24df80..822d70d 100644
>> --- a/src/glsl/glsl_lexer.ll
>> +++ b/src/glsl/glsl_lexer.ll
>> @@ -337,6 +337,7 @@ samplerExternalOES {
>> return IDENTIFIER;
>> }
>>
>> +atomic_uint KEYWORD_WITH_ALT(420, 300, 420, 0, yyextra->ARB_shader_atomic_counters_enable, ATOMIC_UINT);
>>
>> struct return STRUCT;
>> void return VOID_TOK;
>> @@ -518,7 +519,6 @@ restrict KEYWORD(0, 300, 0, 0, RESTRICT);
>> readonly KEYWORD(0, 300, 0, 0, READONLY);
>> writeonly KEYWORD(0, 300, 0, 0, WRITEONLY);
>> resource KEYWORD(0, 300, 0, 0, RESOURCE);
>> -atomic_uint KEYWORD(0, 300, 0, 0, ATOMIC_UINT);
>> patch KEYWORD(0, 300, 0, 0, PATCH);
>> sample KEYWORD(0, 300, 0, 0, SAMPLE);
>> subroutine KEYWORD(0, 300, 0, 0, SUBROUTINE);
>> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
>> index 912931a..fdc7f66 100644
>> --- a/src/glsl/glsl_parser.yy
>> +++ b/src/glsl/glsl_parser.yy
>> @@ -118,6 +118,7 @@ _mesa_glsl_lex(YYSTYPE *val, YYLTYPE *loc, _mesa_glsl_parse_state *state)
>> %token SAMPLER2DMS ISAMPLER2DMS USAMPLER2DMS
>> %token SAMPLER2DMSARRAY ISAMPLER2DMSARRAY USAMPLER2DMSARRAY
>> %token SAMPLEREXTERNALOES
>> +%token ATOMIC_UINT
>> %token STRUCT VOID_TOK WHILE
>> %token <identifier> IDENTIFIER TYPE_IDENTIFIER NEW_IDENTIFIER
>> %type <identifier> any_identifier
>> @@ -147,7 +148,7 @@ _mesa_glsl_lex(YYSTYPE *val, YYLTYPE *loc, _mesa_glsl_parse_state *state)
>> %token HVEC2 HVEC3 HVEC4 DVEC2 DVEC3 DVEC4 FVEC2 FVEC3 FVEC4
>> %token SAMPLER3DRECT
>> %token SIZEOF CAST NAMESPACE USING
>> -%token COHERENT RESTRICT READONLY WRITEONLY RESOURCE ATOMIC_UINT PATCH SAMPLE
>> +%token COHERENT RESTRICT READONLY WRITEONLY RESOURCE PATCH SAMPLE
>> %token SUBROUTINE
>>
>> %token ERROR_TOK
>> @@ -1288,12 +1289,19 @@ layout_qualifier_id:
>> }
>> }
>>
>> - if (state->ARB_shading_language_420pack_enable &&
>> + if ((state->ARB_shading_language_420pack_enable ||
>> + state->ARB_shader_atomic_counters_enable) &&
>> strcmp("binding", $1) == 0) {
>> $$.flags.q.explicit_binding = 1;
>> $$.binding = $3;
>> }
>>
>> + if (state->ARB_shader_atomic_counters_enable &&
>> + strcmp("offset", $1) == 0) {
>> + $$.flags.q.explicit_offset = 1;
>> + $$.offset = $3;
>> + }
>> +
>> if (strcmp("max_vertices", $1) == 0) {
>> $$.flags.q.max_vertices = 1;
>>
>> @@ -1663,6 +1671,7 @@ basic_type_specifier_nonarray:
>> | SAMPLER2DMSARRAY { $$ = "sampler2DMSArray"; }
>> | ISAMPLER2DMSARRAY { $$ = "isampler2DMSArray"; }
>> | USAMPLER2DMSARRAY { $$ = "usampler2DMSArray"; }
>> + | ATOMIC_UINT { $$ = "atomic_uint"; }
>> ;
>>
>> precision_qualifier:
>> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
>> index 0a6a09a..d3e1a78 100644
>> --- a/src/glsl/glsl_parser_extras.h
>> +++ b/src/glsl/glsl_parser_extras.h
>> @@ -31,6 +31,7 @@
>> #ifdef __cplusplus
>>
>>
>> +#include <map>
>> #include <stdlib.h>
>> #include "glsl_symbol_table.h"
>>
>> @@ -331,6 +332,9 @@ struct _mesa_glsl_parse_state {
>> * Unused for other shader types.
>> */
>> unsigned gs_input_size;
>> +
>> + /** Atomic counter offsets by binding */
>> + std::map<unsigned, unsigned> atomic_counter_offsets;
>> };
>>
>> # define YYLLOC_DEFAULT(Current, Rhs, N) \
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131023/d25c422b/attachment.pgp>
More information about the mesa-dev
mailing list