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
>> 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
