<div dir="ltr">On 21 January 2014 04:19, Timothy Arceri <span dir="ltr"><<a href="mailto:t_arceri@yahoo.com.au" target="_blank">t_arceri@yahoo.com.au</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">Signed-off-by: Timothy Arceri <<a href="mailto:t_arceri@yahoo.com.au">t_arceri@yahoo.com.au</a>><br>
---<br>
src/glsl/ast.h | 44 +++++++----<br>
src/glsl/ast_array_index.cpp | 13 ++++<br>
src/glsl/ast_to_hir.cpp | 160 +++++++++++++++++++++++++++-------------<br>
src/glsl/ast_type.cpp | 8 +-<br>
src/glsl/glsl_parser_extras.cpp | 30 ++++----<br>
src/glsl/glsl_parser_extras.h | 2 +<br>
6 files changed, 167 insertions(+), 90 deletions(-)<br>
<br>
diff --git a/src/glsl/ast.h b/src/glsl/ast.h<br>
index 76911f0..4dda32e 100644<br>
--- a/src/glsl/ast.h<br>
+++ b/src/glsl/ast.h<br>
@@ -276,6 +276,21 @@ private:<br>
bool cons;<br>
};<br>
<br>
+class ast_array_specifier : public ast_node {<br>
+public:<br>
+ ast_array_specifier()<br>
+ : ast_node()<br></blockquote><div><br></div><div>Note: it's not necessary to explicitly mention the base class in the initializer list when its constructor takes no arguments.<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">
+ {<br>
+ dimension_count = 1;<br>
+ }<br></blockquote><div><br></div><div>It would be nice to have the constructor(s) of ast_array_specifier initialize it to a consistent state. As written, it creates an ast_array_specifier with a dimension count of 1, but with is_unsized_array set to false and an empty array_dimensions list, which isn't sensible.<br>
<br>I'd recommend having two constructors:<br><br></div><div>/** Unsized array specifier ([]) */<br></div><div>explicit ast_array_specifier(const struct YYLTYPE &locp)<br></div><div> : dimension_count(1), is_unsized_array(true)<br>
{<br></div><div> set_location(locp);<br></div><div>}<br><br></div><div>and one for a sized array:<br><br></div><div>/** Sized array specifier ([dim]) */<br></div><div>ast_array_specifier(ast_expression *dim)<br></div><div>
: dimension_count(1), is_unsized_array(false)<br>{<br></div><div> set_location(locp);<br></div><div> array_dimensions.push_tail(&dim->link);<br>}<br><br></div><div>This would have the added advantage of making it easier for a reader of the class definition to understand how the data structure works.<br>
<br><br></div>It would also be nice to have a method which adds a dimension to the array specifier and maintains the proper invariant on dimension_count:<br><br></div><div class="gmail_quote">void add_dimension(ast_expression *dim)<br>
{<br></div><div class="gmail_quote"> array_dimensions.push_tail(&dim->link);<br></div><div class="gmail_quote"> dimension_count++;<br>}<br><br></div><div class="gmail_quote">That way all the logic for maintaining internal consistency of the data structure is here inside the class definition, rather than in the parser where it's harder to verify that it's correct.<br>
</div><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">
+<br>
+ virtual void print(void) const;<br>
+<br>
+ unsigned dimension_count;<br></blockquote><div><br></div><div>It would be nice to have a comment above dimension_count explaining that this includes both sized and unsized dimensions.<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">
+ bool is_unsized_array;<br></blockquote><div><br></div><div>It would be nice to have a comment above is_unsized_array explaining that if true, this means that the array has an unsized outermost dimension.<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">
+ exec_list array_dimensions;<br></blockquote><div><br></div><div>It would be nice to have a comment explaining that this list contains objects of type ast_node containing the sized dimensions only, in outermost-to-innermost order.<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">
+}; <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
/**<br>
* C-style aggregate initialization class<br>
*<br>
@@ -325,14 +340,15 @@ public:<br>
<br>
class ast_declaration : public ast_node {<br>
public:<br>
- ast_declaration(const char *identifier, bool is_array, ast_expression *array_size,<br>
- ast_expression *initializer);<br>
+ ast_declaration(const char *identifier, bool is_array,<br>
+ ast_array_specifier *array_specifier,<br>
+ ast_expression *initializer);<br>
virtual void print(void) const;<br>
<br>
const char *identifier;<br>
<br>
bool is_array;<br>
- ast_expression *array_size;<br>
+ ast_array_specifier *array_specifier;<br></blockquote><div><br></div><div>Previously the reason we needed is_array was because we used array_size == NULL to represent both non-arrays and unsized arrays. Now that we use a non-NULL array_specifier to represent an unsized array, is_array is redundant--it should be true exactly when array_specifier != NULL. To avoid redunancy, I think we should drop is_array from this class entirely.<br>
<br></div><div>The same goes for the is_array fields of ast_type_specifier, ast_parameter_declarator, and ast_interface_block.<br><br>If you wanted to remove all of that in a follow up patch, that would be fine by me.<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">
<br>
ast_expression *initializer;<br>
};<br>
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp<br>
index 4cc8eb1..326aa58 100644<br>
--- a/src/glsl/ast_to_hir.cpp<br>
+++ b/src/glsl/ast_to_hir.cpp<br>
@@ -1771,63 +1771,116 @@ ast_compound_statement::hir(exec_list *instructions,<br>
return NULL;<br>
}<br>
<br>
+static const unsigned<br>
+process_array_size(exec_node *node,<br>
+ struct _mesa_glsl_parse_state *state)<br></blockquote><div><br></div><div>Since it's hard to infer what this function does from its type signature, I'd recommend adding a comment above it saying something like "Evaluate the given exec_node (which should be an ast_node representing a single array dimension) and return its integer value."<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">
+{<br>
+ int result = 0;<br></blockquote><div><br></div><div>This should have type "unsigned" for consistency with the function signature and the use of "size->value.u[0]" below.<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">
+ exec_list dummy_instructions;<br>
+<br>
+ ast_node *array_size = exec_node_data(ast_node, node, link);<br>
+ ir_rvalue *const ir = array_size->hir(& dummy_instructions,<br>
+ state);<br>
+ YYLTYPE loc = array_size->get_location();<br>
+<br>
+ if (ir != NULL) {<br>
+ if (!ir->type->is_integer()) {<br>
+ _mesa_glsl_error(& loc, state,<br>
+ "array size must be integer type");<br>
+ } else if (!ir->type->is_scalar()) {<br>
+ _mesa_glsl_error(& loc, state,<br>
+ "array size must be scalar type");<br>
+ } else {<br>
+ ir_constant *const size = ir->constant_expression_value();<br>
+<br>
+ if (size == NULL) {<br>
+ _mesa_glsl_error(& loc, state, "array size must be a "<br>
+ "constant valued expression");<br>
+ } else if (size->value.i[0] <= 0) {<br>
+ _mesa_glsl_error(& loc, state, "array size must be > 0");<br>
+ } else {<br>
+ assert(size->type == ir->type);<br>
+ result = size->value.u[0];<br>
+<br>
+ /* If the array size is const (and we've verified that<br>
+ * it is) then no instructions should have been emitted<br>
+ * when we converted it to HIR. If they were emitted,<br>
+ * then either the array size isn't const after all, or<br>
+ * we are emitting unnecessary instructions.<br>
+ */<br>
+ assert(dummy_instructions.is_empty());<br>
+ }<br>
+ }<br>
+ }<br>
+ return result;<br></blockquote><div><br></div><div>Nit pick: it's a bit difficult to follow the nesting of error condition checking here. You might want to consider doing something like this instead:<br><br></div>
<div>if (ir == NULL) {<br></div><div> _mesa_glsl_error(...);<br></div><div> return 0;<br>}<br></div><div>if (!ir->type->is_integer()) {<br></div><div> _mesa_glsl_error(...);<br></div><div> return 0;<br>}<br>
</div><div>if (!ir->type->is_scalar()) {<br></div><div> _mesa_glsl_error(...);<br></div><div> return 0;<br>}<br></div><div>ir_constant * const size = ...;<br></div><div>if (size == NULL) {<br></div><div> _mesa_glsl_error(...);<br>
</div><div> return 0;<br>}<br></div><div>if (size->value.i[0] <= 0) {<br></div><div> _mesa_glsl_error(...);<br></div><div> return 0;<br>}<br></div><div>assert(size->type == ir->type);<br><br></div><div>/* ...no instructions should have been emitted... */<br>
</div><div>assert(dummy_instructions.is_empty());<br></div><div><br></div><div>return size->value.u[0];<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">
+}<br>
<br>
static const glsl_type *<br>
-process_array_type(YYLTYPE *loc, const glsl_type *base, ast_node *array_size,<br>
- struct _mesa_glsl_parse_state *state)<br>
+process_array_type(YYLTYPE *loc, const glsl_type *base,<br>
+ ast_array_specifier *array_specifier,<br>
+ struct _mesa_glsl_parse_state *state)<br>
{<br>
- unsigned length = 0;<br>
+ const glsl_type *array_type = NULL;<br>
+ const glsl_type *element_type = base;<br>
+ const glsl_type *array_type_temp = element_type;<br>
<br>
if (base == NULL)<br>
return glsl_type::error_type;<br></blockquote><div><br></div><div>I think the only way base could ever be NULL here is if there is a bug elsewhere in Mesa--does that seem correct to you? IMHO it would be fine to drop the NULL check entirely. If you really want to be careful, I would recommend adding an assertion so that it's clear that this is not a situation that's expected to occur, and so that the bug will be easier to track down if it ever does:<br>
<br></div><div>if (base == NULL) {<br></div><div> assert(!"NULL unexpectedly passed to process_array_type()");<br></div><div> return glsl_type::error_type;<br>}<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">
<br>
- /* From page 19 (page 25) of the GLSL 1.20 spec:<br>
- *<br>
- * "Only one-dimensional arrays may be declared."<br>
- */<br>
if (base->is_array()) {<br>
- _mesa_glsl_error(loc, state,<br>
- "invalid array of `%s' (only one-dimensional arrays "<br>
- "may be declared)",<br>
- base->name);<br>
- return glsl_type::error_type;<br>
+<br>
+ /* From page 19 (page 25) of the GLSL 1.20 spec:<br>
+ *<br>
+ * "Only one-dimensional arrays may be declared."<br>
+ */<br>
+ if (!state->ARB_arrays_of_arrays_enable) {<br>
+ _mesa_glsl_error(loc, state,<br>
+ "invalid array of `%s'"<br>
+ "#version 120 / GL_ARB_arrays_of_arrays "<br>
+ "required for defining arrays of arrays",<br></blockquote><div><br></div><div>As in patch 2, I think we should drop mention of version 120 from the error message.<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">
+ base->name);<br>
+ return glsl_type::error_type;<br>
+ }<br>
+<br>
+ if (base->length == 0) {<br>
+ _mesa_glsl_error(loc, state,<br>
+ "only the outermost array dimension can "<br>
+ "be unsized",<br>
+ base->name);<br>
+ return glsl_type::error_type;<br>
+ }<br>
+<br>
+ /* Settings this variable will cause<br>
+ * any array dimensions processed with the base type to<br>
+ * be appended onto the end of the array<br>
+ */ <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ element_type = base->element_type();<br></blockquote><div><br><div>This comment is misleading. The only thing element_type is used
for later in the function is to decide whether to set array_type to
NULL, which in turn determines whether we return array_type or
error_type. And for reasons I hope to clarify below, I don't think we
need that logic anyhow. I'd prefer to just drop the element_type variable entirely.<br></div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
}<br>
<br>
- if (array_size != NULL) {<br>
- exec_list dummy_instructions;<br>
- ir_rvalue *const ir = array_size->hir(& dummy_instructions, state);<br>
- YYLTYPE loc = array_size->get_location();<br>
+ if (array_specifier != NULL) {<br></blockquote><div><br></div><div>AFAICT from looking at the callers, this function will never be called with a NULL value of array_specifier. I would recommend dropping this if-test to make the function easier to follow.<br>
<br></div><div>If you really want to be careful you could always add "|| array_specifier == NULL" to the assertion check I mentioned earlier. But IMHO it's not necessary.<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">
<br>
- if (ir != NULL) {<br>
- if (!ir->type->is_integer()) {<br>
- _mesa_glsl_error(& loc, state, "array size must be integer type");<br>
- } else if (!ir->type->is_scalar()) {<br>
- _mesa_glsl_error(& loc, state, "array size must be scalar type");<br>
- } else {<br>
- ir_constant *const size = ir->constant_expression_value();<br>
-<br>
- if (size == NULL) {<br>
- _mesa_glsl_error(& loc, state, "array size must be a "<br>
- "constant valued expression");<br>
- } else if (size->value.i[0] <= 0) {<br>
- _mesa_glsl_error(& loc, state, "array size must be > 0");<br>
- } else {<br>
- assert(size->type == ir->type);<br>
- length = size->value.u[0];<br>
-<br>
- /* If the array size is const (and we've verified that<br>
- * it is) then no instructions should have been emitted<br>
- * when we converted it to HIR. If they were emitted,<br>
- * then either the array size isn't const after all, or<br>
- * we are emitting unnecessary instructions.<br>
- */<br>
- assert(dummy_instructions.is_empty());<br>
- }<br>
- }<br>
+ exec_node *node = array_specifier->array_dimensions.tail_pred;<br>
+<br>
+ unsigned array_size;<br></blockquote><div><br></div><div>Since array_size is only used inside the loop, let's declare it where it's initialized.<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">
+ for (/* nothing */; !node->is_head_sentinel(); node = node->prev) {<br></blockquote><div><br></div><div>Since node is only used inside the loop, we can declare it in the for statement, like this:<br><br>for (exec_node *node = array_specifier->array_dimensions.tail_pred; ...)<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">
+ array_size = process_array_size(node, state);<br>
+ array_type_temp = glsl_type::get_array_instance(array_type_temp,<br>
+ array_size);<br>
+ }<br>
+<br>
+ if (array_specifier->is_unsized_array) {<br>
+ array_type_temp = glsl_type::get_array_instance(array_type_temp,<br>
+ 0);<br>
+ }<br>
+<br>
+ if (array_type_temp == element_type) {<br>
+ /* we found no array sizes */<br>
+ array_type = NULL;<br>
+ } else {<br>
+ array_type = array_type_temp;<br>
}<br></blockquote><div><br></div><div>Since array_specifier is never NULL, and ast_array_specifiers only have an empty array_dimensions list when is_unsized_array is true, array_temp will never equal element_type when we get to this code. I think we should drop this check, and the logic in the return statement below, and simply return array_type_temp. If we do that, we should also be able to drop array_type and then rename array_type_temp to array_type.<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">
}<br>
<br>
- const glsl_type *array_type = glsl_type::get_array_instance(base, length);<br>
return array_type != NULL ? array_type : glsl_type::error_type;<br>
}<br></blockquote><div><br></div><div>With all those changes I think we can simplify the function down to this:<br><br>{<br> if (base->is_array()) {<br><br> /* From page 19 (page 25) of the GLSL 1.20 spec:<br>
*<br> * "Only one-dimensional arrays may be declared."<br> */<br> if (!state->ARB_arrays_of_arrays_enable) {<br> _mesa_glsl_error(loc, state,<br> "invalid array of `%s'"<br>
"GL_ARB_arrays_of_arrays "<br> "required for defining arrays of arrays",<br> base->name);<br> return glsl_type::error_type;<br>
}<br><br> if (base->length == 0) {<br> _mesa_glsl_error(loc, state,<br> "only the outermost array dimension can "<br> "be unsized",<br>
base->name);<br> return glsl_type::error_type;<br> }<br> }<br><br> const glsl_type *array_type = base;<br><br> for (exec_node *node = array_specifier->array_dimensions.tail_pred;<br>
!node->is_head_sentinel(); node = node->prev) {<br> unsigned array_size = process_array_size(node, state);<br> array_type = glsl_type::get_array_instance(array_type, array_size);<br> }<br><br> if (array_specifier->is_unsized_array)<br>
array_type = glsl_type::get_array_instance(array_type, 0);<br><br> return array_type;<br>}<br><br><br></div><div>Here's one other possible simplifying idea: the callers of process_array_type() have logic that either looks like this or could be modified to look like this with suitable refactoring:<br>
<br></div><div>if (this->is_array)<br></div><div> type = process_array_type(&loc, type, this->array_specifier, state);<br><br></div><div>If you take my suggestions above to drop is_array, then that will change to:<br>
<br></div><div>if (this->array_specifier != NULL)<br></div><div> type = process_array_type(&loc, type, this->array_specifier, state);<br><br></div><div>At which point we could modify process_array_type() to simply return base in the case where array_specifier is NULL. If we do that, the call site simplifies to just:<br>
<br>type = process_array_type(&loc, type, this->array_specifier, state);<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">
<br>
@@ -5038,7 +5095,7 @@ ast_interface_block::hir(exec_list *instructions,<br>
* interface array size *doesn't* need to be specified is on a<br>
* geometry shader input.<br>
*/<br>
- if (this->array_size == NULL &&<br>
+ if (block_array_type->is_unsized_array() &&<br></blockquote><div><br></div><div>An alternative that wouldn't require moving the declaration of block_array_type would be to change this to:<br><br>
</div><div>if (this->array_specifier->is_unsized_array && ...<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">
(state->stage != MESA_SHADER_GEOMETRY || !this-><a href="http://layout.flags.q.in" target="_blank">layout.flags.q.in</a>)) {<br>
_mesa_glsl_error(&loc, state,<br>
"only geometry shader inputs may be unsized "<br>
<br></blockquote></div></div></div>