<div dir="ltr">On 29 October 2013 16:37, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><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"><div class="im">v2: Mark atomic counters as read-only variables. Move offset overlap<br>
code to the linker. Use the contains_atomic() convenience method.<br>
</div>v3: Use pointer to integer instead of non-const reference. Add<br>
comment so we remember to add a spec quotation from the next GLSL<br>
release once the issue of atomic counter aggregation within<br>
structures is clarified.<br>
---<br>
src/glsl/ast.h | 15 +++++++++++<br>
src/glsl/ast_to_hir.cpp | 60 ++++++++++++++++++++++++++++++++++++++++++-<br>
<div class="im"> src/glsl/ast_type.cpp | 13 ++++++++--<br>
src/glsl/glsl_lexer.ll | 2 +-<br>
src/glsl/glsl_parser.yy | 13 ++++++++--<br>
src/glsl/glsl_parser_extras.h | 4 +++<br>
</div> 6 files changed, 101 insertions(+), 6 deletions(-)<br>
<div><div class="h5"><br>
diff --git a/src/glsl/ast.h b/src/glsl/ast.h<br>
index 97905c6..5c214b6 100644<br>
--- a/src/glsl/ast.h<br>
+++ b/src/glsl/ast.h<br>
@@ -385,6 +385,12 @@ struct ast_type_qualifier {<br>
*/<br>
unsigned explicit_binding:1;<br>
<br>
+ /**<br>
+ * Flag set if GL_ARB_shader_atomic counter "offset" layout<br>
+ * qualifier is used.<br>
+ */<br>
+ unsigned explicit_offset:1;<br>
+<br>
/** \name Layout qualifiers for GL_AMD_conservative_depth */<br>
/** \{ */<br>
unsigned depth_any:1;<br>
@@ -448,6 +454,15 @@ struct ast_type_qualifier {<br>
int binding;<br>
<br>
/**<br>
+ * Offset specified via GL_ARB_shader_atomic_counter's "offset"<br>
+ * keyword.<br>
+ *<br>
+ * \note<br>
+ * This field is only valid if \c explicit_offset is set.<br>
+ */<br>
+ int offset;<br>
+<br>
+ /**<br>
* Return true if and only if an interpolation qualifier is present.<br>
*/<br>
bool has_interpolation() const;<br>
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp<br>
</div></div>index 71e57ec..306633e 100644<br>
--- a/src/glsl/ast_to_hir.cpp<br>
+++ b/src/glsl/ast_to_hir.cpp<br>
@@ -1997,10 +1997,19 @@ validate_binding_qualifier(struct _mesa_glsl_parse_state *state,<br>
<div class="im"><br>
return false;<br>
}<br>
+ } else if (var->type->contains_atomic()) {<br>
+ if (unsigned(qual->binding) >= ctx->Const.MaxAtomicBufferBindings) {<br>
+ _mesa_glsl_error(loc, state, "layout(binding = %d) exceeds the "<br>
+ " maximum number of atomic counter buffer bindings"<br>
+ "(%d)", qual->binding,<br>
+ ctx->Const.MaxAtomicBufferBindings);<br>
+<br>
+ return false;<br>
+ }<br>
} else {<br>
_mesa_glsl_error(loc, state,<br>
"the \"binding\" qualifier only applies to uniform "<br>
- "blocks, samplers, or arrays of samplers");<br>
+ "blocks, samplers, atomic counters, or arrays thereof");<br>
return false;<br>
}<br>
<br>
</div>@@ -2284,6 +2293,29 @@ apply_type_qualifier_to_variable(const struct ast_type_qualifier *qual,<br>
<div class="im"> var->binding = qual->binding;<br>
}<br>
<br>
+ if (var->type->contains_atomic()) {<br>
+ if (var->mode == ir_var_uniform) {<br>
+ if (var->explicit_binding) {<br>
</div>+ unsigned *offset = &state->atomic_counter_offsets[var->binding];<br>
+<br>
+ if (*offset % ATOMIC_COUNTER_SIZE)<br>
<div class="im">+ _mesa_glsl_error(loc, state,<br>
+ "misaligned atomic counter offset");<br>
+<br>
</div>+ var->atomic.offset = *offset;<br>
+ *offset += var->type->atomic_size();<br>
<div class="im">+<br>
+ } else {<br>
+ _mesa_glsl_error(loc, state,<br>
+ "atomic counters require explicit binding point");<br>
+ }<br>
+ } else if (var->mode != ir_var_function_in) {<br>
+ _mesa_glsl_error(loc, state, "atomic counters may only be declared as "<br>
+ "function parameters or uniform-qualified "<br>
+ "global variables");<br>
+ }<br>
+ }<br>
+<br>
/* Does the declaration use the deprecated 'attribute' or 'varying'<br>
* keywords?<br>
*/<br>
</div>@@ -2819,6 +2851,18 @@ ast_declarator_list::hir(exec_list *instructions,<br>
<div class="im"> (void) this->type->specifier->hir(instructions, state);<br>
<br>
decl_type = this->type->glsl_type(& type_name, state);<br>
+<br>
+ /* An offset-qualified atomic counter declaration sets the default<br>
+ * offset for the next declaration within the same atomic counter<br>
+ * buffer.<br>
+ */<br>
+ if (decl_type && decl_type->contains_atomic()) {<br>
+ if (type->qualifier.flags.q.explicit_binding &&<br>
+ type->qualifier.flags.q.explicit_offset)<br>
+ state->atomic_counter_offsets[type->qualifier.binding] =<br>
+ type->qualifier.offset;<br>
+ }<br>
+<br>
if (this->declarations.is_empty()) {<br>
/* If there is no structure involved in the program text, there are two<br>
* possible scenarios:<br>
</div>@@ -2848,6 +2892,11 @@ ast_declarator_list::hir(exec_list *instructions,<br>
<div class="im"> _mesa_glsl_error(&loc, state,<br>
"invalid type `%s' in empty declaration",<br>
type_name);<br>
+ } else if (decl_type->base_type == GLSL_TYPE_ATOMIC_UINT) {<br>
+ /* Empty atomic counter declarations are allowed and useful<br>
+ * to set the default offset qualifier.<br>
+ */<br>
+ return NULL;<br>
} else if (this->type->qualifier.precision != ast_precision_none) {<br>
if (this->type->specifier->structure != NULL) {<br>
_mesa_glsl_error(&loc, state,<br>
</div>@@ -4549,6 +4598,15 @@ ast_process_structure_or_interface_block(exec_list *instructions,<br>
<div class="im"> "uniform in non-default uniform block contains sampler");<br>
}<br>
<br>
</div>+ if (field_type->contains_atomic()) {<br>
+ /* FINISHME: Add a spec quotation here once updated spec language<br>
+ * FINISHME: is available.<br>
+ */<br></blockquote><div><br></div><div>Is there a Khronos bug for this? If so we should cite the bug number. <br></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">
<div><div class="h5">+ YYLTYPE loc = decl_list->get_location();<br>
+ _mesa_glsl_error(&loc, state, "atomic counter in structure or "<br>
+ "uniform block");<br>
+ }<br>
+<br>
const struct ast_type_qualifier *const qual =<br>
& decl_list->type->qualifier;<br>
if (qual->flags.q.std140 ||<br>
diff --git a/src/glsl/ast_type.cpp b/src/glsl/ast_type.cpp<br>
index 8aabd95..2b088bf 100644<br>
--- a/src/glsl/ast_type.cpp<br>
+++ b/src/glsl/ast_type.cpp<br>
@@ -72,7 +72,8 @@ ast_type_qualifier::has_layout() const<br>
|| this->flags.q.packed<br>
|| this->flags.q.explicit_location<br>
|| this->flags.q.explicit_index<br>
- || this->flags.q.explicit_binding;<br>
+ || this->flags.q.explicit_binding<br>
+ || this->flags.q.explicit_offset;<br>
}<br>
<br>
bool<br>
@@ -121,13 +122,18 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,<br>
ubo_layout_mask.flags.q.packed = 1;<br>
ubo_layout_mask.flags.q.shared = 1;<br>
<br>
+ ast_type_qualifier ubo_binding_mask;<br>
+ ubo_binding_mask.flags.q.explicit_binding = 1;<br>
+ ubo_binding_mask.flags.q.explicit_offset = 1;<br>
+<br>
/* Uniform block layout qualifiers get to overwrite each<br>
* other (rightmost having priority), while all other<br>
* qualifiers currently don't allow duplicates.<br>
*/<br>
<br>
if ((this->flags.i & q.flags.i & ~(ubo_mat_mask.flags.i |<br>
- ubo_layout_mask.flags.i)) != 0) {<br>
+ ubo_layout_mask.flags.i |<br>
+ ubo_binding_mask.flags.i)) != 0) {<br>
_mesa_glsl_error(loc, state,<br>
"duplicate layout qualifiers used");<br>
return false;<br>
@@ -168,6 +174,9 @@ ast_type_qualifier::merge_qualifier(YYLTYPE *loc,<br>
if (q.flags.q.explicit_binding)<br>
this->binding = q.binding;<br>
<br>
+ if (q.flags.q.explicit_offset)<br>
+ this->offset = q.offset;<br>
+<br>
if (q.precision != ast_precision_none)<br>
this->precision = q.precision;<br>
<br>
diff --git a/src/glsl/glsl_lexer.ll b/src/glsl/glsl_lexer.ll<br>
index e24df80..822d70d 100644<br>
--- a/src/glsl/glsl_lexer.ll<br>
+++ b/src/glsl/glsl_lexer.ll<br>
@@ -337,6 +337,7 @@ samplerExternalOES {<br>
return IDENTIFIER;<br>
}<br>
<br>
+atomic_uint KEYWORD_WITH_ALT(420, 300, 420, 0, yyextra->ARB_shader_atomic_counters_enable, ATOMIC_UINT);<br>
<br>
struct return STRUCT;<br>
void return VOID_TOK;<br>
@@ -518,7 +519,6 @@ restrict KEYWORD(0, 300, 0, 0, RESTRICT);<br>
readonly KEYWORD(0, 300, 0, 0, READONLY);<br>
writeonly KEYWORD(0, 300, 0, 0, WRITEONLY);<br>
resource KEYWORD(0, 300, 0, 0, RESOURCE);<br>
-atomic_uint KEYWORD(0, 300, 0, 0, ATOMIC_UINT);<br>
patch KEYWORD(0, 300, 0, 0, PATCH);<br>
sample KEYWORD(0, 300, 0, 0, SAMPLE);<br>
subroutine KEYWORD(0, 300, 0, 0, SUBROUTINE);<br>
diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy<br>
</div></div>index 4ed4105..d0aff12 100644<br>
--- a/src/glsl/glsl_parser.yy<br>
+++ b/src/glsl/glsl_parser.yy<br>
@@ -144,6 +144,7 @@ static bool match_layout_qualifier(const char *s1, const char *s2,<br>
<div class="im"> %token SAMPLER2DMS ISAMPLER2DMS USAMPLER2DMS<br>
%token SAMPLER2DMSARRAY ISAMPLER2DMSARRAY USAMPLER2DMSARRAY<br>
%token SAMPLEREXTERNALOES<br>
+%token ATOMIC_UINT<br>
%token STRUCT VOID_TOK WHILE<br>
%token <identifier> IDENTIFIER TYPE_IDENTIFIER NEW_IDENTIFIER<br>
%type <identifier> any_identifier<br>
</div>@@ -173,7 +174,7 @@ static bool match_layout_qualifier(const char *s1, const char *s2,<br>
<div class="im"> %token HVEC2 HVEC3 HVEC4 DVEC2 DVEC3 DVEC4 FVEC2 FVEC3 FVEC4<br>
%token SAMPLER3DRECT<br>
%token SIZEOF CAST NAMESPACE USING<br>
-%token COHERENT RESTRICT READONLY WRITEONLY RESOURCE ATOMIC_UINT PATCH SAMPLE<br>
+%token COHERENT RESTRICT READONLY WRITEONLY RESOURCE PATCH SAMPLE<br>
%token SUBROUTINE<br>
<br>
%token ERROR_TOK<br>
</div>@@ -1328,12 +1329,19 @@ layout_qualifier_id:<br>
<div class="im"> }<br>
}<br>
<br>
- if (state->ARB_shading_language_420pack_enable &&<br>
+ if ((state->ARB_shading_language_420pack_enable ||<br>
+ state->ARB_shader_atomic_counters_enable) &&<br>
</div> match_layout_qualifier("binding", $1, state) == 0) {<br>
<div class="im"> $$.flags.q.explicit_binding = 1;<br>
$$.binding = $3;<br>
}<br>
<br>
+ if (state->ARB_shader_atomic_counters_enable &&<br>
</div>+ match_layout_qualifier("offset", $1, state) == 0) {<br>
<div class="im">+ $$.flags.q.explicit_offset = 1;<br>
+ $$.offset = $3;<br>
+ }<br>
+<br>
</div> if (match_layout_qualifier("max_vertices", $1, state) == 0) {<br>
$$.flags.q.max_vertices = 1;<br>
<br>
@@ -1707,6 +1715,7 @@ basic_type_specifier_nonarray:<br>
<div class="im"> | SAMPLER2DMSARRAY { $$ = "sampler2DMSArray"; }<br>
| ISAMPLER2DMSARRAY { $$ = "isampler2DMSArray"; }<br>
| USAMPLER2DMSARRAY { $$ = "usampler2DMSArray"; }<br>
+ | ATOMIC_UINT { $$ = "atomic_uint"; }<br>
;<br>
<br>
precision_qualifier:<br>
diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h<br>
</div>index f3560c3..0cc5ecf 100644<br>
<div class="im">--- a/src/glsl/glsl_parser_extras.h<br>
+++ b/src/glsl/glsl_parser_extras.h<br>
@@ -31,6 +31,7 @@<br>
#ifdef __cplusplus<br>
<br>
<br>
+#include <map><br>
#include <stdlib.h><br>
#include "glsl_symbol_table.h"<br>
<br>
</div>@@ -351,6 +352,9 @@ struct _mesa_glsl_parse_state {<br>
<div class="im"> * Unused for other shader types.<br>
*/<br>
unsigned gs_input_size;<br>
+<br>
+ /** Atomic counter offsets by binding */<br>
+ std::map<unsigned, unsigned> atomic_counter_offsets;<br></div></blockquote><div><br></div><div>This patch is:<br><br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
</div><div><br>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:<br>
<br>- <a href="http://lists.freedesktop.org/archives/mesa-dev/2010-September/002930.html">http://lists.freedesktop.org/archives/mesa-dev/2010-September/002930.html</a> ("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).")<br><br>- <a href="http://lists.freedesktop.org/archives/mesa-dev/2012-February/019299.html">http://lists.freedesktop.org/archives/mesa-dev/2012-February/019299.html</a> ("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).")<br>
<br>- <a href="http://lists.freedesktop.org/archives/mesa-dev/2013-September/044869.html">http://lists.freedesktop.org/archives/mesa-dev/2013-September/044869.html</a> ("No. We are not using STL or templates.")<br>
<br></div><div>(Please feel free to reply with additional links if I'm missing something significant.)<br></div><div><br>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.<br>
</div><br><br></div>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.<br>
<div class="gmail_quote"><div><br>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 <a href="http://en.wikipedia.org/wiki/Standard_Template_Library#Criticisms">http://en.wikipedia.org/wiki/Standard_Template_Library#Criticisms</a> for a starting point. I would be delighted to participate in such a discussion.<br>
<br></div><div>Once we've had that discussion and reached a resolution, then I think this patch can land.<br></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">
<div class="im">
};<br>
<br>
# define YYLLOC_DEFAULT(Current, Rhs, N) \<br>
</div><span class=""><font color="#888888">--<br>
1.8.3.4<br>
</font></span><div class=""><div class="h5"><br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</div></div></blockquote></div><br></div></div>