[Mesa-dev] [PATCH v2 36/82] glsl: add support for unsized arrays in shader storage blocks

[Samuel Iglesias Gonsálvez]

On 15/06/15 13:25, Timothy Arceri wrote:
> On Wed, 2015-06-03 at 09:01 +0200, Iago Toral Quiroga wrote:
>> From: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>
>> They only can be defined in the last position of the shader
>> storage blocks.
>>
>> When an unsized array is used in different shaders, it might be
>> converted in different sized arrays, avoid get a linker error
>> in that case.
>>
>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>> ---
>>  src/glsl/ast_array_index.cpp |   6 +--
>>  src/glsl/ast_to_hir.cpp      |  32 +++++++++++++
>>  src/glsl/ir.cpp              |   1 +
>>  src/glsl/ir.h                |  14 ++++++
>>  src/glsl/linker.cpp          | 107 ++++++++++++++++++++++++++++---------------
>>  5 files changed, 121 insertions(+), 39 deletions(-)
>>
>> diff --git a/src/glsl/ast_array_index.cpp b/src/glsl/ast_array_index.cpp
>> index 752d86f..481efae 100644
>> --- a/src/glsl/ast_array_index.cpp
>> +++ b/src/glsl/ast_array_index.cpp
>> @@ -106,7 +106,6 @@ update_max_array_access(ir_rvalue *ir, int idx, YYLTYPE *loc,
>>     }
>>  }
>>  
>> -
>>  ir_rvalue *
>>  _mesa_ast_array_index_to_hir(void *mem_ctx,
>>  			     struct _mesa_glsl_parse_state *state,
>> @@ -182,8 +181,9 @@ _mesa_ast_array_index_to_hir(void *mem_ctx,
>>        if (array->type->is_array())
>>           update_max_array_access(array, idx, &loc, state);
>>     } else if (const_index == NULL && array->type->is_array()) {
>> -      if (array->type->is_unsized_array()) {
>> -	 _mesa_glsl_error(&loc, state, "unsized array index must be constant");
>> +      if (array->type->is_unsized_array() &&
>> +          array->variable_referenced()->data.mode != ir_var_shader_storage) {
>> +            _mesa_glsl_error(&loc, state, "unsized array index must be constant");
>>        } else if (array->type->fields.array->is_interface()
>>                   && array->variable_referenced()->data.mode == ir_var_uniform
>>                   && !state->is_version(400, 0) && !state->ARB_gpu_shader5_enable) {
>> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
>> index e1e5ca9..3ec28dd 100644
>> --- a/src/glsl/ast_to_hir.cpp
>> +++ b/src/glsl/ast_to_hir.cpp
>> @@ -5507,6 +5507,19 @@ private:
>>     bool found;
>>  };
>>  
>> +static bool
>> +is_unsized_array_last_element(ir_variable *v)
>> +{
>> +   const glsl_type *interface_type = v->get_interface_type();
>> +   int length = interface_type->length;
>> +
>> +   assert(v->type->is_unsized_array());
>> +
>> +   /* Check if it is the last element of the interface */
>> +   if (strcmp(interface_type->fields.structure[length-1].name, v->name) == 0)
>> +      return true;
>> +   return false;
>> +}
>>  
>>  ir_rvalue *
>>  ast_interface_block::hir(exec_list *instructions,
>> @@ -5828,6 +5841,15 @@ ast_interface_block::hir(exec_list *instructions,
>>           var->data.explicit_binding = this->layout.flags.q.explicit_binding;
>>           var->data.binding = this->layout.binding;
>>  
>> +         if (var->is_in_shader_storage_block() && var->type->is_unsized_array()) {
>> +            var->data.from_ssbo_unsized_array = true;
>> +            if (!is_unsized_array_last_element(var)) {
>> +               _mesa_glsl_error(&loc, state, "unsized array `%s' should be the"
>> +                                 " last member of a shader storage block",
>> +                                 var->name);
>> +            }
>> +         }
> 
> The existing comment above this is misleading I think its a copy and
> paste error from the non instance name block code below.
> 
> Anyway to get to the point here you are testing if the block itself is
> an unsized array not the member inside the block.
> 

Yes, you are right. I am going to modify it.

> Also when gles both this test and the test below should throw an error
> when there is an unsized array member but its not in a shader storage
> block.
> 
> "If an array is declared as the last member of a shader storage block
> and the size is not specified at compile-time, it is sized at run-time.
> In all other cases,  arrays are sized only at compile-time."
> 

Good catch! I am going to fix it too.

Thanks for the review!

Sam

> 
>> +
>>           state->symbols->add_variable(var);
>>           instructions->push_tail(var);
>>        }
>> @@ -5900,6 +5922,16 @@ ast_interface_block::hir(exec_list *instructions,
>>           var->data.explicit_binding = this->layout.flags.q.explicit_binding;
>>           var->data.binding = this->layout.binding;
>>  
>> +         if (var->data.mode == ir_var_shader_storage && var->type->is_unsized_array()) {
>> +            var->data.from_ssbo_unsized_array = true;
>> +            if (var->is_in_shader_storage_block() &&
>> +                !is_unsized_array_last_element(var)) {
>> +               _mesa_glsl_error(&loc, state, "unsized array `%s' should be the"
>> +                                 " last member of a shader storage block",
>> +                                 var->name);
>> +            }
>> +         }
>> +
>>           state->symbols->add_variable(var);
>>           instructions->push_tail(var);
>>        }
> 
> 
>