Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
On 2015-08-05 01:30:08, Iago Toral Quiroga wrote:
> From: Samuel Iglesias Gonsalvez <sigles...@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 Arceri)
>
> Signed-off-by: Samuel Iglesias Gonsalvez <sigles...@igalia.com>
> ---
> src/glsl/ast_array_index.cpp | 3 +-
> 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, 154 insertions(+), 37 deletions(-)
>
> diff --git a/src/glsl/ast_array_index.cpp b/src/glsl/ast_array_index.cpp
> index 27e84d1..115b5f2 100644
> --- a/src/glsl/ast_array_index.cpp
> +++ b/src/glsl/ast_array_index.cpp
> @@ -226,7 +226,8 @@ _mesa_ast_array_index_to_hir(void *mem_ctx,
> * by the linker.
> */
> }
> - else {
> + else if (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()
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 68f71c6..6e63d86 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -5844,6 +5844,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,
> @@ -6210,6 +6223,33 @@ ast_interface_block::hir(exec_list *instructions,
> else if (state->stage == MESA_SHADER_TESS_CTRL && var_mode ==
> ir_var_shader_out)
> handle_tess_ctrl_shader_output_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."
> + */
> + 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) {
> @@ -6301,6 +6341,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 724861b..594fc33 100644
> --- a/src/glsl/ir.cpp
> +++ b/src/glsl/ir.cpp
> @@ -1657,6 +1657,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 ede8caa..822a8a3 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
> *
> @@ -778,6 +787,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 a16dab4..9aff3ce 100644
> --- a/src/glsl/linker.cpp
> +++ b/src/glsl/linker.cpp
> @@ -877,30 +877,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;
> @@ -959,12 +969,24 @@ cross_validate_globals(struct gl_shader_program *prog,
> && existing->type->record_compare(var->type)) {
> existing->type = var->type;
> } else {
> - 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
> + * ignore the array size.
> + */
> + 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;
> + }
> }
> }
> }
> @@ -1391,12 +1413,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);
> }
> @@ -1405,7 +1429,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);
> }
> @@ -1446,9 +1471,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);
> @@ -1491,14 +1517,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
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
