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

Timothy Arceri t_arceri at yahoo.com.au
Wed Oct 14 20:43:00 PDT 2015


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.


More information about the mesa-dev mailing list