[Mesa-dev] [PATCH v3 (part2) 08/56] glsl: add support for unsized arrays in shader storage blocks
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Mon Aug 24 06:19:07 PDT 2015
On 05/08/15 08:41, Iago Toral wrote:
> On Tue, 2015-08-04 at 14:08 -0700, Jordan Justen wrote:
>> On 2015-07-14 00:46:10, 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.
>>>
>>> v2:
>>> - Rework error condition and error messages (Timothy Arteri)
>>
>> Arceri
>>
>>> Signed-off-by: Samuel Iglesias Gonsalvez <siglesias at igalia.com>
>>> ---
>>> src/glsl/ast_array_index.cpp | 5 +-
>>> src/glsl/ast_to_hir.cpp | 66 ++++++++++++++++++++++++++
>>> src/glsl/ir.cpp | 1 +
>>> src/glsl/ir.h | 14 ++++++
>>> src/glsl/linker.cpp | 107 ++++++++++++++++++++++++++++---------------
>>> 5 files changed, 155 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/src/glsl/ast_array_index.cpp b/src/glsl/ast_array_index.cpp
>>> index 2c79002..8a1ae67 100644
>>> --- a/src/glsl/ast_array_index.cpp
>>> +++ b/src/glsl/ast_array_index.cpp
>>> @@ -182,8 +182,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 ca30dbc..1b4ee22 100644
>>> --- a/src/glsl/ast_to_hir.cpp
>>> +++ b/src/glsl/ast_to_hir.cpp
>>> @@ -5599,6 +5599,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,
>>> @@ -5913,6 +5926,33 @@ ast_interface_block::hir(exec_list *instructions,
>>> if (state->stage == MESA_SHADER_GEOMETRY && var_mode == ir_var_shader_in)
>>> handle_geometry_shader_input_decl(state, loc, var);
>>>
>>> + for (unsigned i = 0; i < num_variables; i++) {
>>> + if (fields[i].type->is_unsized_array()) {
>>> + if (var_mode == ir_var_shader_storage) {
>>> + if (i != (num_variables - 1)) {
>>> + _mesa_glsl_error(&loc, state, "unsized array `%s' definition: "
>>> + "only last member of a shader storage block "
>>> + "can be defined as unsized array",
>>> + fields[i].name);
>>> + }
>>> + } else {
>>> + /* From GLSL ES 3.10 spec, section 4.1.9 "Arrays":
>>> + *
>>> + * "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."
>>> + */
>>
>> Where is the check for 'last member' in the es path?
>>
>> Is this es check new? If so, should the es check be added in a patch
>> before this one, and then extended for SSBO support?
>>
>>> + if (state->es_shader) {
>>> + _mesa_glsl_error(&loc, state, "unsized array `%s' definition: "
>>> + "only last member of a shader storage block "
>>> + "can be defined as unsized array",
>>> + fields[i].name);
>>> + }
>>> + }
>>> + }
>>> + }
>>> +
>>>
>>> if (ir_variable *earlier =
>>> state->symbols->get_variable(this->instance_name)) {
>>> if (!redeclaring_per_vertex) {
>>> @@ -6003,6 +6043,32 @@ 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->type->is_unsized_array()) {
>>> + if (var->is_in_shader_storage_block()) {
>>> + if (!is_unsized_array_last_element(var)) {
>>> + _mesa_glsl_error(&loc, state, "unsized array `%s' definition: "
>>> + "only last member of a shader storage block "
>>> + "can be defined as unsized array",
>>> + var->name);
>>> + }
>>> + var->data.from_ssbo_unsized_array = true;
>>> + } else {
>>> + /* From GLSL ES 3.10 spec, section 4.1.9 "Arrays":
>>> + *
>>> + * "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."
>>> + */
>>> + if (state->es_shader) {
>>> + _mesa_glsl_error(&loc, state, "unsized array `%s' definition: "
>>> + "only last member of a shader storage block "
>>> + "can be defined as unsized array",
>>> + var->name);
>>> + }
>>> + }
>>> + }
>>> +
>>> state->symbols->add_variable(var);
>>> instructions->push_tail(var);
>>> }
>>> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
>>> index 9a25bf4..390e8f3 100644
>>> --- a/src/glsl/ir.cpp
>>> +++ b/src/glsl/ir.cpp
>>> @@ -1654,6 +1654,7 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name,
>>> this->data.image_coherent = false;
>>> this->data.image_volatile = false;
>>> this->data.image_restrict = false;
>>> + this->data.from_ssbo_unsized_array = false;
>>>
>>> if (type != NULL) {
>>> if (type->base_type == GLSL_TYPE_SAMPLER)
>>> diff --git a/src/glsl/ir.h b/src/glsl/ir.h
>>> index 1c7829b..e677b89 100644
>>> --- a/src/glsl/ir.h
>>> +++ b/src/glsl/ir.h
>>> @@ -453,6 +453,15 @@ public:
>>> }
>>>
>>> /**
>>> + * Determine whether or not a variable is part of a shader storage block.
>>> + */
>>> + inline bool is_in_shader_storage_block() const
>>> + {
>>> + return this->data.mode == ir_var_shader_storage &&
>>> + this->interface_type != NULL;
>>> + }
>>> +
>>> + /**
>>> * Determine whether or not a variable is the declaration of an interface
>>> * block
>>> *
>>> @@ -777,6 +786,11 @@ public:
>>> unsigned image_restrict:1;
>>>
>>> /**
>>> + * ARB_shader_storage_buffer_object
>>> + */
>>> + unsigned from_ssbo_unsized_array:1; /**< unsized array buffer variable. */
>>> +
>>> + /**
>>> * Emit a warning if this variable is accessed.
>>> */
>>> private:
>>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>>> index 28ad2f3..ed490d0 100644
>>> --- a/src/glsl/linker.cpp
>>> +++ b/src/glsl/linker.cpp
>>> @@ -724,30 +724,40 @@ validate_intrastage_arrays(struct gl_shader_program *prog,
>>> * In addition, set the type of the linked variable to the
>>> * explicitly sized array.
>>> */
>>> - if (var->type->is_array() && existing->type->is_array() &&
>>> - (var->type->fields.array == existing->type->fields.array) &&
>>> - ((var->type->length == 0)|| (existing->type->length == 0))) {
>>> - if (var->type->length != 0) {
>>> - if (var->type->length <= existing->data.max_array_access) {
>>> - linker_error(prog, "%s `%s' declared as type "
>>> - "`%s' but outermost dimension has an index"
>>> - " of `%i'\n",
>>> - mode_string(var),
>>> - var->name, var->type->name,
>>> - existing->data.max_array_access);
>>> - }
>>> - existing->type = var->type;
>>> - return true;
>>> - } else if (existing->type->length != 0) {
>>> - if(existing->type->length <= var->data.max_array_access) {
>>> - linker_error(prog, "%s `%s' declared as type "
>>> - "`%s' but outermost dimension has an index"
>>> - " of `%i'\n",
>>> - mode_string(var),
>>> - var->name, existing->type->name,
>>> - var->data.max_array_access);
>>> + if (var->type->is_array() && existing->type->is_array()) {
>>> + if ((var->type->fields.array == existing->type->fields.array) &&
>>> + ((var->type->length == 0)|| (existing->type->length == 0))) {
>>> + if (var->type->length != 0) {
>>> + if (var->type->length <= existing->data.max_array_access) {
>>> + linker_error(prog, "%s `%s' declared as type "
>>> + "`%s' but outermost dimension has an index"
>>> + " of `%i'\n",
>>> + mode_string(var),
>>> + var->name, var->type->name,
>>> + existing->data.max_array_access);
>>> + }
>>> + existing->type = var->type;
>>> + return true;
>>> + } else if (existing->type->length != 0) {
>>> + if(existing->type->length <= var->data.max_array_access &&
>>> + !existing->data.from_ssbo_unsized_array) {
>>> + linker_error(prog, "%s `%s' declared as type "
>>> + "`%s' but outermost dimension has an index"
>>> + " of `%i'\n",
>>> + mode_string(var),
>>> + var->name, existing->type->name,
>>> + var->data.max_array_access);
>>> + }
>>> + return true;
>>> }
>>> - return true;
>>> + } else {
>>> + /* The arrays of structs could have different glsl_type pointers but
>>> + * they are actually the same type. Use record_compare() to check that.
>>> + */
>>> + if (existing->type->fields.array->is_record() &&
>>> + var->type->fields.array->is_record() &&
>>> + existing->type->fields.array->record_compare(var->type->fields.array))
>>> + return true;
>>> }
>>> }
>>> return false;
>>> @@ -802,12 +812,24 @@ cross_validate_globals(struct gl_shader_program *prog,
>>> && existing->type->record_compare(var->type)) {
>>> existing->type = var->type;
>>> } else {
>>
>> Looks like you could use else if.
>
> Yes, however I think Samuel did it like this so he could place the
> comment before the actual condition. Otherwise he would have to move the
> else if to a new line and put the comment above, in between the
> closing } for the if and the beginning of the else if... since the
> comment spans over a few lines, I think it is probably easier to read as
> it is. The other option would be to place the comment inside the if, but
> that would be odd, since the comment explains what the if is testing
> for, so it should really be before the condition.
>
> Anyway, let me know if you still prefer the else if option.
>
As Iago pointed out, it was done in this way to improve readability of
the code. If you think it is wrong, I will change it.
Sam
>>> - linker_error(prog, "%s `%s' declared as type "
>>> - "`%s' and type `%s'\n",
>>> - mode_string(var),
>>> - var->name, var->type->name,
>>> - existing->type->name);
>>> - return;
>>> + /* If it is an unsized array in a Shader Storage Block,
>>> + * two different shaders can access to different elements.
>>> + * Because of that, they might be converted to different
>>> + * sized arrays, then check that they are compatible but
>>> + * forget about the array size.
>>> + */
>>
>> How about "ignore" in place of "forget about"?
>>
>> -Jordan
>>
>>> + if (!(var->data.mode == ir_var_shader_storage &&
>>> + var->data.from_ssbo_unsized_array &&
>>> + existing->data.mode == ir_var_shader_storage &&
>>> + existing->data.from_ssbo_unsized_array &&
>>> + var->type->gl_type == existing->type->gl_type)) {
>>> + linker_error(prog, "%s `%s' declared as type "
>>> + "`%s' and type `%s'\n",
>>> + mode_string(var),
>>> + var->name, var->type->name,
>>> + existing->type->name);
>>> + return;
>>> + }
>>> }
>>> }
>>> }
>>> @@ -1234,12 +1256,14 @@ public:
>>>
>>> virtual ir_visitor_status visit(ir_variable *var)
>>> {
>>> - fixup_type(&var->type, var->data.max_array_access);
>>> + fixup_type(&var->type, var->data.max_array_access,
>>> + var->data.from_ssbo_unsized_array);
>>> if (var->type->is_interface()) {
>>> if (interface_contains_unsized_arrays(var->type)) {
>>> const glsl_type *new_type =
>>> resize_interface_members(var->type,
>>> - var->get_max_ifc_array_access());
>>> + var->get_max_ifc_array_access(),
>>> + var->is_in_shader_storage_block());
>>> var->type = new_type;
>>> var->change_interface_type(new_type);
>>> }
>>> @@ -1248,7 +1272,8 @@ public:
>>> if (interface_contains_unsized_arrays(var->type->fields.array)) {
>>> const glsl_type *new_type =
>>> resize_interface_members(var->type->fields.array,
>>> - var->get_max_ifc_array_access());
>>> + var->get_max_ifc_array_access(),
>>> + var->is_in_shader_storage_block());
>>> var->change_interface_type(new_type);
>>> var->type = update_interface_members_array(var->type, new_type);
>>> }
>>> @@ -1289,9 +1314,10 @@ private:
>>> * If the type pointed to by \c type represents an unsized array, replace
>>> * it with a sized array whose size is determined by max_array_access.
>>> */
>>> - static void fixup_type(const glsl_type **type, unsigned max_array_access)
>>> + static void fixup_type(const glsl_type **type, unsigned max_array_access,
>>> + bool from_ssbo_unsized_array)
>>> {
>>> - if ((*type)->is_unsized_array()) {
>>> + if (!from_ssbo_unsized_array && (*type)->is_unsized_array()) {
>>> *type = glsl_type::get_array_instance((*type)->fields.array,
>>> max_array_access + 1);
>>> assert(*type != NULL);
>>> @@ -1334,14 +1360,23 @@ private:
>>> */
>>> static const glsl_type *
>>> resize_interface_members(const glsl_type *type,
>>> - const unsigned *max_ifc_array_access)
>>> + const unsigned *max_ifc_array_access,
>>> + bool is_ssbo)
>>> {
>>> unsigned num_fields = type->length;
>>> glsl_struct_field *fields = new glsl_struct_field[num_fields];
>>> memcpy(fields, type->fields.structure,
>>> num_fields * sizeof(*fields));
>>> for (unsigned i = 0; i < num_fields; i++) {
>>> - fixup_type(&fields[i].type, max_ifc_array_access[i]);
>>> + /* If SSBO last member is unsized array, we don't replace it by a sized
>>> + * array.
>>> + */
>>> + if (is_ssbo && i == (num_fields - 1))
>>> + fixup_type(&fields[i].type, max_ifc_array_access[i],
>>> + true);
>>> + else
>>> + fixup_type(&fields[i].type, max_ifc_array_access[i],
>>> + false);
>>> }
>>> glsl_interface_packing packing =
>>> (glsl_interface_packing) type->interface_packing;
>>> --
>>> 1.9.1
>>>
>>
>
>
>
More information about the mesa-dev
mailing list