On 15 September 2013 00:10, Francisco Jerez <curroje...@riseup.net> wrote:
> ---
> src/glsl/ast.h | 15 ++++++++++
> src/glsl/ast_to_hir.cpp | 68
> +++++++++++++++++++++++++++++++++++++++++--
> src/glsl/ast_type.cpp | 13 +++++++--
> src/glsl/glsl_lexer.ll | 2 +-
> src/glsl/glsl_parser.yy | 13 +++++++--
> src/glsl/glsl_parser_extras.h | 10 +++++++
> 6 files changed, 114 insertions(+), 7 deletions(-)
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 26c4701..8a5d3fc 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -405,6 +405,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;
> @@ -468,6 +474,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 fcca5df..7edbee4 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -1197,6 +1197,9 @@ ast_expression::hir(exec_list *instructions,
> !state->check_version(120, 300, &loc,
> "array comparisons forbidden")) {
> error_emitted = true;
> + } else if ((op[0]->type->atomic_size() ||
> op[1]->type->atomic_size())) {
> + _mesa_glsl_error(&loc, state, "atomic counter comparisons
> forbidden");
> + error_emitted = true;
>
Do we have spec text to back this up? I looked around and couldn't find
anything. It seems like doing equality comparisons on atomic counters is
ill-defined, though (do two counters compare equal if they have equal
values, or if they point to the same counter? Both possibilities seem
dodgy). Maybe we should file a spec bug so we can get clarification from
khronos about what's intended.
Note that we currently permit other comparisons that are similarly dodgy
(e.g. comparing samplers). It would be nice to get clarification from
khronos about this too.
> }
>
> if (error_emitted) {
> @@ -1952,10 +1955,19 @@ validate_binding_qualifier(struct
> _mesa_glsl_parse_state *state,
>
> return false;
> }
> + } else if (var->type->atomic_size()) {
> + 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;
> }
>
> @@ -1983,7 +1995,7 @@ apply_type_qualifier_to_variable(const struct
> ast_type_qualifier *qual,
> }
>
> if (qual->flags.q.constant || qual->flags.q.attribute
> - || qual->flags.q.uniform
> + || (qual->flags.q.uniform && var->type !=
> glsl_type::atomic_uint_type)
> || (qual->flags.q.varying && (state->target == fragment_shader)))
> var->read_only = 1;
>
I'm not entirely convinced this is right. An atomic counter, like a
sampler, is a handle to an underlying object, not the underlying object
itself. The handle should be considered read-only even if the object it
refers to is mutable. That way we still prohibit
uniform atomic_uint foo;
uniform atomic_uint bar;
void main() {
foo = bar;
}
>
> @@ -2225,6 +2237,35 @@ apply_type_qualifier_to_variable(const struct
> ast_type_qualifier *qual,
> var->binding = qual->binding;
> }
>
> + if (var->type->atomic_size()) {
> + if (var->mode == ir_var_uniform) {
> + if (var->explicit_binding) {
> + _mesa_glsl_parse_state::atomic_counter_binding &binding =
> + state->atomic_counter_bindings[var->binding];
> +
> + if (binding.next_offset % ATOMIC_COUNTER_SIZE)
> + _mesa_glsl_error(loc, state,
> + "misaligned atomic counter offset");
> +
> + if (binding.offsets.count(binding.next_offset))
> + _mesa_glsl_error(loc, state,
> + "atomic counter offsets must be unique");
>
GLSL 4.20 clarifies that atomic counter offsets must be unique and
non-overlapping (see GLSL 4.20 4.4.4.1 "Atomic Counter Layout
Qualifiers"). So I tihnk we need a stronger check than this.
Specifically, declarations like the following should be disallowed:
layout(binding=0, offset=16) atomic_uint foo[4]; // Uses offsets 16, 20,
24, 28
layout(binding=0, offset=20) atomic_uint bar; // Error: overlaps foo.
> +
> + var->atomic.offset = binding.next_offset;
> + binding.offsets.insert(binding.next_offset);
> + binding.next_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?
> */
> @@ -2725,6 +2766,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->atomic_size()) {
> + if (type->qualifier.flags.q.explicit_binding &&
> + type->qualifier.flags.q.explicit_offset)
> + state->atomic_counter_bindings[type->qualifier.binding]
> + .next_offset = type->qualifier.offset;
> + }
> +
> if (this->declarations.is_empty()) {
> /* If there is no structure involved in the program text, there are
> two
> * possible scenarios:
> @@ -2754,6 +2807,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,
> @@ -4475,6 +4533,12 @@ ast_process_structure_or_interface_block(exec_list
> *instructions,
> "uniform in non-default uniform block
> contains sampler");
> }
>
> + if (field_type->atomic_size()) {
> + YYLTYPE loc = decl_list->get_location();
> + _mesa_glsl_error(&loc, state, "atomic counter in structure or
> "
> + "uniform block");
> + }
> +
>
Are you sure this is not allowed? I can't find any spec text to back this
up. All I found was this text from ARB_shader_atomic_counters:
"Except for array indexing, structure field selection, and parenthesis,
counters are not allowed to be operands in expressions."
Which would seem to imply that atomic counters *are* allowed as structure
fields.
> 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 fa6e205..b9498b4 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 4ffbf8f..d0e131a 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -31,6 +31,8 @@
> #ifdef __cplusplus
>
>
> +#include <map>
> +#include <set>
> #include <stdlib.h>
> #include "glsl_symbol_table.h"
>
> @@ -331,6 +333,14 @@ struct _mesa_glsl_parse_state {
> * Unused for other shader types.
> */
> unsigned gs_input_size;
> +
> + struct atomic_counter_binding {
> + atomic_counter_binding() : offsets(), next_offset(0) {}
> + std::set<unsigned> offsets;
> + unsigned next_offset;
> + };
> +
> + std::map<unsigned, atomic_counter_binding> atomic_counter_bindings;
> };
>
> # define YYLLOC_DEFAULT(Current, Rhs, N) \
> --
> 1.8.3.4
>
> _______________________________________________
> 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
