<div dir="ltr">On 15 September 2013 00:10, Francisco Jerez <<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">--- 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; </blockquote><div> </div><div>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. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> } 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; </blockquote><div> </div><div>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 </div><div>uniform atomic_uint foo; </div><div>uniform atomic_uint bar; void main() { </div><div> foo = bar; } </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> @@ -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"); </blockquote><div> </div><div>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: </div><div>layout(binding=0, offset=16) atomic_uint foo[4]; // Uses offsets 16, 20, 24, 28 </div><div>layout(binding=0, offset=20) atomic_uint bar; // Error: overlaps foo. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + + 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"); + } + </blockquote><div> </div><div>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." </div><div>Which would seem to imply that atomic counters *are* allowed as structure fields. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> 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 <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>