[Mesa-dev] [PATCH v3 (part2) 08/56] glsl: add support for unsized arrays in shader storage blocks
Iago Toral
itoral at igalia.com
Wed Aug 5 00:57:16 PDT 2015
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?
The "else" branch here where the if (state->es_shader) is included is
for things that are not SSBOs. What I think Samuel was trying to do here
is to produce an error for any unsized array declaration in the ES path
that is not inside an SSBO (if it is inside the SSBO, then it goes
though the if branch and that checks that it is the last member in the
definition.
If this is what he intended to do, then I agree that it would probably
make sense to have the check included in a separate patch before this
one since it is unrelated to SSBOs, then modify that code with this
patch to add the ssbo path included in the if branch.
I'll let Samuel have a look at this when he is back from holidays, since
since he might have other reasons for doing it like this.
> > + 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.
>
> > - 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