Thank you for your comments, I've rebased my atomic counters branch on top of master [1] and I think I've taken into account everything you had pointed out.
The rebase has turned out to be especially painful this time. To avoid wasting more time rebasing patches that no-one is going to have a look at, I'm going to start merging the ones that have already got any review by the end of this week. Thank you. [1] http://cgit.freedesktop.org/~currojerez/mesa/log/?h=atomic-counters Ian Romanick <i...@freedesktop.org> writes: > 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) \ >>
pgpWhHmnrnO_2.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
