[Mesa-dev] [PATCH 32/34] glsl: Allow geometry shader input instance arrays to be unsized.

[Ian Romanick]

On 07/28/2013 11:03 PM, Paul Berry wrote:
> ---
>   src/glsl/ast.h          | 24 ++++++++++++++++--------
>   src/glsl/ast_to_hir.cpp | 31 +++++++++++++++++++++++++++++--
>   src/glsl/glsl_parser.yy | 11 ++++-------
>   3 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 3ef9913..a40bbc0 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -907,12 +907,14 @@ public:
>   class ast_interface_block : public ast_node {
>   public:
>      ast_interface_block(ast_type_qualifier layout,
> -                     const char *instance_name,
> -		     ast_expression *array_size)
> +                       const char *instance_name,
> +                       bool is_array,
> +                       ast_expression *array_size)
>      : layout(layout), block_name(NULL), instance_name(instance_name),
> -     array_size(array_size)
> +     is_array(is_array), array_size(array_size)
>      {
> -      /* empty */
> +      if (!is_array)
> +         assert(array_size == NULL);

I think this is better as:

         assert(is_array || array_size == NULL);

The otherwise empty if-statements always bug me.

>      }
>
>      virtual ir_rvalue *hir(exec_list *instructions,
> @@ -933,13 +935,19 @@ public:
>      exec_list declarations;
>
>      /**
> -    * Declared array size of the block instance
> -    *
> -    * If the block is not declared as an array, this field will be \c NULL.
> +    * True if the block is declared as an array
>       *
>       * \note
>       * A block can only be an array if it also has an instance name.  If this
> -    * field is not \c NULL, ::instance_name must also not be \c NULL.
> +    * field is true, ::instance_name must also not be \c NULL.
> +    */
> +   bool is_array;
> +
> +   /**
> +    * Declared array size of the block instance, or NULL if the block instance
> +    * array is unsized.

I'd leave the first line as-is and change the second part to be:

    * If the block is not declared as an array or if the block instance
    * array is unsizied, this field will be \c NULL.

That has the added benefit of making the diff look cleaner.

> +    *
> +    * If the block is not declared as an array, this field will be \c NULL.
>       */
>      ast_expression *array_size;
>   };
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 3a013c5..44399c6 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -4396,7 +4396,34 @@ ast_interface_block::hir(exec_list *instructions,
>      if (this->instance_name) {
>         ir_variable *var;
>
> -      if (this->array_size != NULL) {
> +      if (this->is_array) {
> +         /* Section 4.3.7 (Interface Blocks) of the GLSL 1.50 spec says:
> +          *
> +          *     For uniform blocks declared an array, each individual array
> +          *     element corresponds to a separate buffer object backing one
> +          *     instance of the block. As the array size indicates the number
> +          *     of buffer objects needed, uniform block array declarations
> +          *     must specify an array size.
> +          *
> +          * And a few paragraphs later:
> +          *
> +          *     Geometry shader input blocks must be declared as arrays and
> +          *     follow the array declaration and linking rules for all
> +          *     geometry shader inputs. All other input and output block
> +          *     arrays must specify an array size.
> +          *
> +          * The upshot of this is that the only circumstance where an
> +          * interface array size *doesn't* need to be specified is on a
> +          * geometry shader input.
> +          */
> +         if (this->array_size == NULL &&
> +             (state->target != geometry_shader || !this->layout.flags.q.in)) {
> +            _mesa_glsl_error(&loc, state,
> +                             "only geometry shader inputs may be unsized "
> +                             "instance block arrays");
> +
> +         }
> +
>            const glsl_type *block_array_type =
>               process_array_type(&loc, block_type, this->array_size, state);
>
> @@ -4416,7 +4443,7 @@ ast_interface_block::hir(exec_list *instructions,
>         /* In order to have an array size, the block must also be declared with
>          * an instane name.
>          */
> -      assert(this->array_size == NULL);
> +      assert(!this->is_array);
>
>         for (unsigned i = 0; i < num_variables; i++) {
>            ir_variable *var =
> diff --git a/src/glsl/glsl_parser.yy b/src/glsl/glsl_parser.yy
> index 6f39d07..8ebc3e1 100644
> --- a/src/glsl/glsl_parser.yy
> +++ b/src/glsl/glsl_parser.yy
> @@ -2239,25 +2239,22 @@ instance_name_opt:
>      /* empty */
>      {
>         $$ = new(state) ast_interface_block(*state->default_uniform_qualifier,
> -                                          NULL, NULL);
> +                                          NULL, false, NULL);
>      }
>      | NEW_IDENTIFIER
>      {
>         $$ = new(state) ast_interface_block(*state->default_uniform_qualifier,
> -                                          $1, NULL);
> +                                          $1, false, NULL);
>      }
>      | NEW_IDENTIFIER '[' constant_expression ']'
>      {
>         $$ = new(state) ast_interface_block(*state->default_uniform_qualifier,
> -                                          $1, $3);
> +                                          $1, true, $3);
>      }
>      | NEW_IDENTIFIER '[' ']'
>      {
> -      _mesa_glsl_error(& @1, state,
> -                       "instance block arrays must be explicitly sized");
> -
>         $$ = new(state) ast_interface_block(*state->default_uniform_qualifier,
> -                                          $1, NULL);
> +                                          $1, true, NULL);
>      }
>      ;
>
>