[Mesa-dev] [PATCH 1/6] glsl: Implement parser support for atomic counters.
Paul Berry
stereotype441 at gmail.com
Fri Nov 1 08:15:57 PDT 2013
On 29 October 2013 16:37, Francisco Jerez <currojerez at riseup.net> wrote:
> v2: Mark atomic counters as read-only variables. Move offset overlap
> code to the linker. Use the contains_atomic() convenience method.
> v3: Use pointer to integer instead of non-const reference. Add
> comment so we remember to add a spec quotation from the next GLSL
> release once the issue of atomic counter aggregation within
> structures is clarified.
> ---
> src/glsl/ast.h | 15 +++++++++++
> src/glsl/ast_to_hir.cpp | 60
> ++++++++++++++++++++++++++++++++++++++++++-
> 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, 101 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 71e57ec..306633e 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -1997,10 +1997,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;
> }
>
> @@ -2284,6 +2293,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];
> +
> + if (*offset % ATOMIC_COUNTER_SIZE)
> + _mesa_glsl_error(loc, state,
> + "misaligned atomic counter offset");
> +
> + var->atomic.offset = *offset;
> + *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?
> */
> @@ -2819,6 +2851,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:
> @@ -2848,6 +2892,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,
> @@ -4549,6 +4598,15 @@ ast_process_structure_or_interface_block(exec_list
> *instructions,
> "uniform in non-default uniform block
> contains sampler");
> }
>
> + if (field_type->contains_atomic()) {
> + /* FINISHME: Add a spec quotation here once updated spec
> language
> + * FINISHME: is available.
> + */
>
Is there a Khronos bug for this? If so we should cite the bug number.
> + 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 4ed4105..d0aff12 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -144,6 +144,7 @@ static bool match_layout_qualifier(const char *s1,
> const char *s2,
> %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
> @@ -173,7 +174,7 @@ static bool match_layout_qualifier(const char *s1,
> const char *s2,
> %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
> @@ -1328,12 +1329,19 @@ layout_qualifier_id:
> }
> }
>
> - if (state->ARB_shading_language_420pack_enable &&
> + if ((state->ARB_shading_language_420pack_enable ||
> + state->ARB_shader_atomic_counters_enable) &&
> match_layout_qualifier("binding", $1, state) == 0) {
> $$.flags.q.explicit_binding = 1;
> $$.binding = $3;
> }
>
> + if (state->ARB_shader_atomic_counters_enable &&
> + match_layout_qualifier("offset", $1, state) == 0) {
> + $$.flags.q.explicit_offset = 1;
> + $$.offset = $3;
> + }
> +
> if (match_layout_qualifier("max_vertices", $1, state) == 0) {
> $$.flags.q.max_vertices = 1;
>
> @@ -1707,6 +1715,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 f3560c3..0cc5ecf 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"
>
> @@ -351,6 +352,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;
>
This patch is:
Reviewed-by: Paul Berry <stereotype441 at gmail.com>
However, I don't think it's appropriate to land the patch until we resolve
the issue of whether we're going to allow STL in Mesa. Although there has
been a fair amount of hallway discussion about the question at Intel, I
can't find much about the subject here on the Mesa-dev list. Here's what I
have found:
- http://lists.freedesktop.org/archives/mesa-dev/2010-September/002930.html("Also,
I think you should start using the STL: there are several parts of
the compilers that could use a std::map or std::unordered_map and are doing
linear scans instead, resulting in potential catastrophic slowdown in
pathological cases (e.g. finding induction variables).")
- http://lists.freedesktop.org/archives/mesa-dev/2012-February/019299.html("STL
has the ostringstream class for this exact purpose, but that would
require rewriting everything to use C++ style IO, and I'm loathe to do that
(mostly because we don't use it anywhere else).")
- http://lists.freedesktop.org/archives/mesa-dev/2013-September/044869.html("No.
We are not using STL or templates.")
(Please feel free to reply with additional links if I'm missing something
significant.)
I think we need to have a genuine discussion on this subject (and I mean a
discussion based on pros and cons and factual information, not just people
saying "no" or "yes"), and it needs to happen on the mailing list so that
(a) everyone can participate, and (b) what we decide, and our reasons for
deciding it, is recorded in the mailing list archives.
So here's my question: how attached are you to the idea of these patches
landing in time for Mesa 10.0? If you really want them to land in time for
Mesa 10.0, then I think as a practical matter, the only choice is to
rewrite them without using STL, because we're not going to have time to
finish a discussion about STL before the merge window closes.
If you're ok with these patches waiting until after Mesa 10.0, then I'd
recommend sending an RFC email to the Mesa-dev list proposing that we allow
the use of STL, and listing the pros and cons. Include the mailing list
links above (and any others if you find them) so that people can have
context for the discussion. Try to be as even-handed and complete as
possible. If you're having trouble thinking of cons (and I wouldn't blame
you if you are--I'm a fan of STL too) you might have a look at
http://en.wikipedia.org/wiki/Standard_Template_Library#Criticisms for a
starting point. I would be delighted to participate in such a discussion.
Once we've had that discussion and reached a resolution, then I think this
patch can land.
> };
>
> # define YYLLOC_DEFAULT(Current, Rhs, N) \
> --
> 1.8.3.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131101/f59754f9/attachment-0001.html>
More information about the mesa-dev
mailing list