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.
>
> > 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.
> If that it's the case, it seems this hunk can be in a separate patch,
> right?
>
> Sam
>
> > bool allow_inputs = state->stage ==
> > MESA_SHADER_GEOMETRY ||
> > state->stage ==
> > MESA_SHADER_TESS_CTRL ||
> > state->stage ==
> > MESA_SHADER_TESS_EVAL;
> > @@ -6354,9 +6402,6 @@ ast_interface_block::hir(exec_list
> > *instructions,
> > }
> > }
> >
> > - const glsl_type *block_array_type =
> > - process_array_type(&loc, block_type, this
> > ->array_specifier, state);
> > -
> > /* From section 4.3.9 (Interface Blocks) of the GLSL ES
> > 3.10 spec:
> > *
> > * * Arrays of arrays of blocks are not allowed
> > diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> > index c1bcccc..16c9171 100644
> > --- a/src/glsl/glsl_parser.yy
> > +++ b/src/glsl/glsl_parser.yy
> > @@ -1962,7 +1962,9 @@ array_specifier:
> > '[' ']'
> > {
> > void *ctx = state;
> > - $$ = new(ctx) ast_array_specifier(@1);
> > + $$ = new(ctx) ast_array_specifier(@1, new(ctx)
> > ast_expression(
> > +
> > ast_unsized_array_dim, NULL,
> > + NULL, NULL));
> > $$->set_location_range(@1, @2);
> > }
> > | '[' constant_expression ']'
> > @@ -1973,17 +1975,16 @@ array_specifier:
> > }
> > | array_specifier '[' ']'
> > {
> > + void *ctx = state;
> > $$ = $1;
> >
> > if (!state->ARB_arrays_of_arrays_enable) {
> > _mesa_glsl_error(& @1, state,
> > "GL_ARB_arrays_of_arrays "
> > "required for defining arrays of
> > arrays");
> > - } else {
> > - _mesa_glsl_error(& @1, state,
> > - "only the outermost array dimension can
> > "
> > - "be unsized");
> > }
> > + $$->add_dimension(new(ctx)
> > ast_expression(ast_unsized_array_dim, NULL,
> > + NULL, NULL));
> > }
> > | array_specifier '[' constant_expression ']'
> > {
> >
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
