[Mesa-dev] [PATCH v2 03/14] glsl: Implement parser support for atomic counters.
Ian Romanick
idr at freedesktop.org
Tue Oct 22 14:42:05 PDT 2013
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) \
>
More information about the mesa-dev
mailing list