[Mesa-dev] [PATCH V7 03/24] glsl: allow AoA to be sized by initializer or constructor
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Wed Oct 14 23:03:16 PDT 2015
On 15/10/15 05:43, Timothy Arceri wrote:
> 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 <siglesias at 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.
>
Add my R-b to it too.
Thanks,
Sam
More information about the mesa-dev
mailing list