On 09/17/2013 03:51 PM, Paul Berry wrote:
> On 15 September 2013 00:10, Francisco Jerez <curroje...@riseup.net
> <mailto: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
Section 4.1.7 (Opaque Types) of the GLSL 4.40 spec says:
"Except for array indexing, structure member selection, and
parentheses, opaque variables are not allowed to be operands
in expressions; such use results in a compile-time error."
We shouldn't allow comparisons of samplers either. :)
> 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.
Given the above spec quote, I think the intention is clear. We're just
doing the wrong thing. I'm starting to feel better about my is_opaque /
contains_opaque predicate idea...
> }
>
> 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
Yeah, I was going to say the same thing. :)
> 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.
I also think this check belongs in the linker. I don't see a lot of
value in doing it twice, and we have to verify atomics declared in other
compilation units.
> +
> + 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:
Yeah, this is definitely wrong. See the text I quoted in reply to patch
8. However, opaque types are not allowed in uniform blocks. Section
4.3.9 (Interface Blocks) of the GLSL 4.40 spec specifically calls that out:
"Types and declarators are the same as for other input, output,
uniform, and buffer variable declarations outside blocks, with
these exceptions:
• initializers are not allowed
• opaque types are not allowed
• structure definitions cannot be nested inside a block"
> "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 <mailto: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
