On Fri, 2015-10-09 at 13:33 +0200, Samuel Iglesias Gonsálvez wrote: > > On 09/10/15 13:25, Timothy Arceri wrote: > > On Thu, 2015-10-08 at 11:08 +0200, Samuel Iglesias Gonsálvez wrote: > > > On 07/10/15 00:47, Timothy Arceri wrote: > > > > From Section 4.1.9 of the GLSL ES 3.10 spec: > > > > > > > > "Arrays are sized either at compile-time or at run-time. > > > > To size an array at compile-time, either the size > > > > must be specified within the brackets as above or > > > > must be inferred from the type of the initializer." > > > > --- > > > > src/glsl/ast.h | 15 ++------- > > > > src/glsl/ast_array_index.cpp | 7 ++-- > > > > src/glsl/ast_function.cpp | 33 +++++++++++++++++- > > > > src/glsl/ast_to_hir.cpp | 79 > > > > ++++++++++++++++++++++++++++++++++---------- > > > > src/glsl/glsl_parser.yy | 11 +++--- > > > > 5 files changed, 104 insertions(+), 41 deletions(-) > > > > > > > > diff --git a/src/glsl/ast.h b/src/glsl/ast.h > > > > index 4c31436..b43be24 100644 > > > > --- a/src/glsl/ast.h > > > > +++ b/src/glsl/ast.h > > > > @@ -181,6 +181,7 @@ enum ast_operators { > > > > ast_post_dec, > > > > ast_field_selection, > > > > ast_array_index, > > > > + ast_unsized_array_dim, > > > > > > > > ast_function_call, > > > > > > > > @@ -318,16 +319,7 @@ public: > > > > > > > > class ast_array_specifier : public ast_node { > > > > public: > > > > - /** Unsized array specifier ([]) */ > > > > - explicit ast_array_specifier(const struct YYLTYPE &locp) > > > > - : is_unsized_array(true) > > > > - { > > > > - set_location(locp); > > > > - } > > > > - > > > > - /** Sized array specifier ([dim]) */ > > > > ast_array_specifier(const struct YYLTYPE &locp, > > > > ast_expression > > > > *dim) > > > > - : is_unsized_array(false) > > > > { > > > > set_location(locp); > > > > array_dimensions.push_tail(&dim->link); > > > > @@ -340,11 +332,8 @@ public: > > > > > > > > virtual void print(void) const; > > > > > > > > - /* If true, this means that the array has an unsized > > > > outermost > > > > dimension. */ > > > > - bool is_unsized_array; > > > > - > > > > /* This list contains objects of type ast_node containing > > > > the > > > > - * sized dimensions only, in outermost-to-innermost order. > > > > + * array dimensions in outermost-to-innermost order. > > > > */ > > > > exec_list array_dimensions; > > > > }; > > > > diff --git a/src/glsl/ast_array_index.cpp > > > > b/src/glsl/ast_array_index.cpp > > > > index 5e8f49d..7855e0a 100644 > > > > --- a/src/glsl/ast_array_index.cpp > > > > +++ b/src/glsl/ast_array_index.cpp > > > > @@ -28,13 +28,10 @@ > > > > void > > > > ast_array_specifier::print(void) const > > > > { > > > > - if (this->is_unsized_array) { > > > > - printf("[ ] "); > > > > - } > > > > - > > > > foreach_list_typed (ast_node, array_dimension, link, &this > > > > ->array_dimensions) { > > > > printf("[ "); > > > > - array_dimension->print(); > > > > + if (((ast_expression*)array_dimension)->oper != > > > > ast_unsized_array_dim) > > > > + array_dimension->print(); > > > > printf("] "); > > > > } > > > > } > > > > diff --git a/src/glsl/ast_function.cpp > > > > b/src/glsl/ast_function.cpp > > > > index 26d4c62..cf4e64a 100644 > > > > --- a/src/glsl/ast_function.cpp > > > > +++ b/src/glsl/ast_function.cpp > > > > @@ -950,6 +950,7 @@ process_array_constructor(exec_list > > > > *instructions, > > > > } > > > > > > > > bool all_parameters_are_constant = true; > > > > + const glsl_type *element_type = constructor_type > > > > ->fields.array; > > > > > > > > /* Type cast each parameter and, if possible, fold > > > > constants. > > > > */ > > > > foreach_in_list_safe(ir_rvalue, ir, &actual_parameters) { > > > > @@ -976,12 +977,34 @@ process_array_constructor(exec_list > > > > *instructions, > > > > } > > > > } > > > > > > > > - if (result->type != constructor_type->fields.array) { > > > > + if (constructor_type->fields.array->is_unsized_array()) > > > > { > > > > + /* As the inner parameters of the constructor are > > > > created > > > > without > > > > + * knowledge of each other we need to check to make > > > > sure > > > > unsized > > > > + * parameters of unsized constructors all end up with > > > > the > > > > same size. > > > > + * > > > > + * e.g we make sure to fail for a constructor like > > > > this: > > > > + * vec4[][] a = vec4[][](vec4[](vec4(0.0), > > > > vec4(1.0)), > > > > + * vec4[](vec4(0.0), vec4(1.0), > > > > vec4(1.0)), > > > > + * vec4[](vec4(0.0), > > > > vec4(1.0))); > > > > + */ > > > > + if (element_type->is_unsized_array()) { > > > > + /* This is the first parameter so just get the > > > > type > > > > */ > > > > + element_type = result->type; > > > > + } else if (element_type != result->type) { > > > > + _mesa_glsl_error(loc, state, "type error in array > > > > constructor: " > > > > + "expected: %s, found %s", > > > > + element_type->name, > > > > + result->type->name); > > > > + return ir_rvalue::error_value(ctx); > > > > + } > > > > + } else if (result->type != constructor_type > > > > ->fields.array) { > > > > _mesa_glsl_error(loc, state, "type error in array > > > > constructor: " > > > > "expected: %s, found %s", > > > > constructor_type->fields.array > > > > ->name, > > > > result->type->name); > > > > return ir_rvalue::error_value(ctx); > > > > + } else { > > > > + element_type = result->type; > > > > } > > > > > > > > /* Attempt to convert the parameter to a constant valued > > > > expression. > > > > @@ -998,6 +1021,14 @@ process_array_constructor(exec_list > > > > *instructions, > > > > ir->replace_with(result); > > > > } > > > > > > > > + if (constructor_type->fields.array->is_unsized_array()) { > > > > + constructor_type = > > > > + glsl_type::get_array_instance(element_type, > > > > + parameter_count); > > > > + assert(constructor_type != NULL); > > > > + assert(constructor_type->length == parameter_count); > > > > + } > > > > + > > > > if (all_parameters_are_constant) > > > > return new(ctx) ir_constant(constructor_type, > > > > &actual_parameters); > > > > > > > > diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp > > > > index 9511440..3dfa3e6 100644 > > > > --- a/src/glsl/ast_to_hir.cpp > > > > +++ b/src/glsl/ast_to_hir.cpp > > > > @@ -782,8 +782,30 @@ validate_assignment(struct > > > > _mesa_glsl_parse_state *state, > > > > * Note: Whole-array assignments are not permitted in GLSL > > > > 1.10, but this > > > > * is handled by ir_dereference::is_lvalue. > > > > */ > > > > - if (lhs->type->is_unsized_array() && rhs->type->is_array() > > > > - && (lhs->type->fields.array == rhs->type > > > > ->fields.array)) { > > > > + const glsl_type *lhs_t = lhs->type; > > > > + const glsl_type *rhs_t = rhs->type; > > > > + bool unsized_array = false; > > > > + while(lhs_t->is_array()) { > > > > + if (rhs_t == lhs_t) > > > > + break; /* the rest of the inner arrays match so break > > > > out > > > > early */ > > > > + if (!rhs_t->is_array()) { > > > > + unsized_array = false; > > > > + break; /* number of dimensions mismatch */ > > > > + } > > > > + if (lhs_t->length == rhs_t->length) { > > > > + lhs_t = lhs_t->fields.array; > > > > + rhs_t = rhs_t->fields.array; > > > > + continue; > > > > + } else if (lhs_t->is_unsized_array()) { > > > > + unsized_array = true; > > > > + } else { > > > > + unsized_array = false; > > > > + break; /* sized array mismatch */ > > > > + } > > > > + lhs_t = lhs_t->fields.array; > > > > + rhs_t = rhs_t->fields.array; > > > > + } > > > > + if (unsized_array) { > > > > if (is_initializer) { > > > > return rhs; > > > > } else { > > > > @@ -1804,6 +1826,10 @@ ast_expression::do_hir(exec_list > > > > *instructions, > > > > break; > > > > } > > > > > > > > + case ast_unsized_array_dim: > > > > + assert(!"ast_unsized_array_dim: Should never get > > > > here."); > > > > + break; > > > > + > > > > case ast_function_call: > > > > /* Should *NEVER* get here. ast_function_call should > > > > always > > > > be handled > > > > * by ast_function_expression::hir. > > > > @@ -1967,6 +1993,14 @@ process_array_size(exec_node *node, > > > > exec_list dummy_instructions; > > > > > > > > ast_node *array_size = exec_node_data(ast_node, node, > > > > link); > > > > + > > > > + /** > > > > + * Dimensions other than the outermost dimension can by > > > > unsized > > > > if they > > > > > > s/by/be > > > > > > > + * are immediately sized by a constructor or initializer. > > > > + */ > > > > + if (((ast_expression*)array_size)->oper == > > > > ast_unsized_array_dim) > > > > + return 0; > > > > + > > > > ir_rvalue *const ir = array_size->hir(& dummy_instructions, > > > > state); > > > > YYLTYPE loc = array_size->get_location(); > > > > > > > > @@ -2035,14 +2069,6 @@ process_array_type(YYLTYPE *loc, const > > > > glsl_type *base, > > > > base->name); > > > > return glsl_type::error_type; > > > > } > > > > - > > > > - if (base->length == 0) { > > > > - _mesa_glsl_error(loc, state, > > > > - "only the outermost array > > > > dimension > > > > can " > > > > - "be unsized", > > > > - base->name); > > > > - return glsl_type::error_type; > > > > - } > > > > } > > > > > > > > for (exec_node *node = array_specifier > > > > ->array_dimensions.tail_pred; > > > > @@ -2050,9 +2076,6 @@ process_array_type(YYLTYPE *loc, const > > > > glsl_type *base, > > > > unsigned array_size = process_array_size(node, > > > > state); > > > > array_type = > > > > glsl_type::get_array_instance(array_type, > > > > array_size); > > > > } > > > > - > > > > - if (array_specifier->is_unsized_array) > > > > - array_type = > > > > glsl_type::get_array_instance(array_type, > > > > 0); > > > > } > > > > > > > > return array_type; > > > > @@ -2591,6 +2614,25 @@ > > > > is_conflicting_fragcoord_redeclaration(struct > > > > _mesa_glsl_parse_state *state, > > > > return false; > > > > } > > > > > > > > +static inline void > > > > +validate_array_dimensions(const glsl_type *t, > > > > + struct _mesa_glsl_parse_state > > > > *state, > > > > + YYLTYPE *loc) { > > > > + if (t->is_array()) { > > > > + t = t->fields.array; > > > > + while (t->is_array()) { > > > > + if (t->is_unsized_array()) { > > > > + _mesa_glsl_error(loc, state, > > > > + "only the outermost array > > > > dimension > > > > can " > > > > + "be unsized", > > > > + t->name); > > > > + break; > > > > + } > > > > + t = t->fields.array; > > > > + } > > > > + } > > > > +} > > > > + > > > > > > It seems the changes related to of array dimensions could be in a > > > separate patch. > > > > Thanks for all the reviews :) > > > > Yes I can split the validation out into its own patch. I thin > > preceding > > this one. > > > > OK, thanks. > > > > > > > > > > static void > > > > apply_type_qualifier_to_variable(const struct > > > > ast_type_qualifier > > > > *qual, > > > > ir_variable *var, > > > > @@ -4264,6 +4306,8 @@ ast_declarator_list::hir(exec_list > > > > *instructions, > > > > result = process_initializer((earlier == NULL) ? var > > > > : > > > > earlier, > > > > decl, this->type, > > > > > > > > &initializer_instructions, > > > > state); > > > > + } else { > > > > + validate_array_dimensions(var_type, state, &loc); > > > > } > > > > > > > > /* From page 23 (page 29 of the PDF) of the GLSL 1.10 > > > > spec: > > > > @@ -5789,6 +5833,7 @@ > > > > ast_process_structure_or_interface_block(exec_list > > > > *instructions, > > > > > > > > const struct glsl_type *field_type = > > > > process_array_type(&loc, decl_type, decl > > > > ->array_specifier, state); > > > > + validate_array_dimensions(field_type, state, &loc); > > > > fields[i].type = field_type; > > > > fields[i].name = decl->identifier; > > > > fields[i].location = -1; > > > > @@ -6304,6 +6349,9 @@ ast_interface_block::hir(exec_list > > > > *instructions, > > > > ir_variable *var; > > > > > > > > if (this->array_specifier != NULL) { > > > > + const glsl_type *block_array_type = > > > > + process_array_type(&loc, block_type, this > > > > ->array_specifier, state); > > > > + > > > > /* Section 4.3.7 (Interface Blocks) of the GLSL 1.50 > > > > spec > > > > says: > > > > * > > > > * For uniform blocks declared an array, each > > > > individual array > > > > @@ -6327,7 +6375,7 @@ ast_interface_block::hir(exec_list > > > > *instructions, > > > > * tessellation control shader output, and > > > > tessellation > > > > evaluation > > > > * shader input. > > > > */ > > > > - if (this->array_specifier->is_unsized_array) { > > > > + if (block_array_type->is_unsized_array()) { > > > > > > > > > I guess this is needed as interface blocks (e.g. uniform blocks) > > > can > > > be > > > arrays of arrays? > > > > > > > No its because this patch removes the is_unsized_array field. This > > code > > is used by single dimension arrays also. > > > > OK, thanks for the explanation. > > This patch (except validation code which would be in a separate one) > is: > > Reviewed-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com> > > Sam
Thanks. Did you want me to post the validation patch to the list or can I just add your r-b to it? I'm planning on pushing a bunch of these patches later today so I dont have to spend so much time rebasing the remaining ones. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev