[Mesa-dev] [PATCH V7 03/24] glsl: allow AoA to be sized by initializer or constructor

Samuel Iglesias Gonsálvez siglesias at igalia.com
Thu Oct 8 02:08:13 PDT 2015


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.

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

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 ']'
>     {
> 


More information about the mesa-dev mailing list