From 5b2675093e863a52b610f112884ae12d42513770 Mon Sep 17 00:00:00 2001 From: Dave Airlie Date: Wed, 25 May 2016 13:31:41 +1000 Subject: [PATCH] glsl: handle implicit sized arrays in ssbo MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit The current code disallows unsized arrays except at the end of an SSBO but it is a bit overzealous in doing so. struct a { int b[]; int f[4]; }; is valid as long as b is implicitly sized within the shader, i.e. it is accessed only by integer indices. I've submitted some piglit tests to test for this. This also has no regressions on piglit on my Haswell. This fixes: GL45-CTS.shader_storage_buffer_object.basic-syntax GL45-CTS.shader_storage_buffer_object.basic-syntaxSSO This patch moves a chunk of the linker code down, so that we don't link the uniform blocks until after we've merged all the variables. The logic went something like: Removing the checks for last ssbo member unsized from the compiler and into the linker, meant doing the check in the link_uniform_blocks code. However to do that the array sizing had to happen first, so we knew that the only unsized arrays were in the last block. But array sizing required the variable to be merged, otherwise you'd get two different array sizes in different version of two variables, and one would get lost when merged. So the solution was to move array sizing up, after variable merging, but before uniform block visiting. Reviewed-by: Samuel Iglesias Gonsálvez Signed-off-by: Dave Airlie --- src/compiler/glsl/ast_to_hir.cpp | 47 +----------- src/compiler/glsl/ir.h | 1 + src/compiler/glsl/ir_validate.cpp | 3 +- src/compiler/glsl/link_uniform_blocks.cpp | 16 +++-- src/compiler/glsl/linker.cpp | 87 +++++++++++++---------- src/compiler/glsl_types.h | 1 + 6 files changed, 66 insertions(+), 89 deletions(-) diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp index b818de6fe1c..22bc0083326 100644 --- a/src/compiler/glsl/ast_to_hir.cpp +++ b/src/compiler/glsl/ast_to_hir.cpp @@ -7484,31 +7484,6 @@ ast_interface_block::hir(exec_list *instructions, 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 (var->data.mode == ir_var_shader_storage) apply_memory_qualifiers(var, fields[i]); } @@ -7624,26 +7599,8 @@ ast_interface_block::hir(exec_list *instructions, 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); + if (is_unsized_array_last_element(var)) { + var->data.from_ssbo_unsized_array = true; } } } diff --git a/src/compiler/glsl/ir.h b/src/compiler/glsl/ir.h index d52dbf8a380..e8efd27112f 100644 --- a/src/compiler/glsl/ir.h +++ b/src/compiler/glsl/ir.h @@ -819,6 +819,7 @@ public: */ unsigned from_ssbo_unsized_array:1; /**< unsized array buffer variable. */ + unsigned implicit_sized_array:1; /** * Emit a warning if this variable is accessed. */ diff --git a/src/compiler/glsl/ir_validate.cpp b/src/compiler/glsl/ir_validate.cpp index 9d05e7e00a9..757f17cbf38 100644 --- a/src/compiler/glsl/ir_validate.cpp +++ b/src/compiler/glsl/ir_validate.cpp @@ -743,7 +743,8 @@ ir_validate::visit(ir_variable *ir) const glsl_struct_field *fields = ir->get_interface_type()->fields.structure; for (unsigned i = 0; i < ir->get_interface_type()->length; i++) { - if (fields[i].type->array_size() > 0) { + if (fields[i].type->array_size() > 0 && + !fields[i].implicit_sized_array) { const int *const max_ifc_array_access = ir->get_max_ifc_array_access(); diff --git a/src/compiler/glsl/link_uniform_blocks.cpp b/src/compiler/glsl/link_uniform_blocks.cpp index ac415b54f03..3c2d13c99c7 100644 --- a/src/compiler/glsl/link_uniform_blocks.cpp +++ b/src/compiler/glsl/link_uniform_blocks.cpp @@ -34,9 +34,10 @@ namespace { class ubo_visitor : public program_resource_visitor { public: ubo_visitor(void *mem_ctx, gl_uniform_buffer_variable *variables, - unsigned num_variables) + unsigned num_variables, struct gl_shader_program *prog) : index(0), offset(0), buffer_size(0), variables(variables), - num_variables(num_variables), mem_ctx(mem_ctx), is_array_instance(false) + num_variables(num_variables), mem_ctx(mem_ctx), is_array_instance(false), + prog(prog) { /* empty */ } @@ -56,6 +57,7 @@ public: unsigned num_variables; void *mem_ctx; bool is_array_instance; + struct gl_shader_program *prog; private: virtual void visit_field(const glsl_type *type, const char *name, @@ -148,7 +150,13 @@ private: */ const glsl_type *type_for_size = type; if (type->is_unsized_array()) { - assert(last_field); + if (!last_field) { + linker_error(prog, "unsized array `%s' definition: " + "only last member of a shader storage block " + "can be defined as unsized array", + name); + } + type_for_size = type->without_array(); } @@ -314,7 +322,7 @@ create_buffer_blocks(void *mem_ctx, struct gl_context *ctx, /* Add each variable from each uniform block to the API tracking * structures. */ - ubo_visitor parcel(blocks, variables, num_variables); + ubo_visitor parcel(blocks, variables, num_variables, prog); STATIC_ASSERT(unsigned(GLSL_INTERFACE_PACKING_STD140) == unsigned(ubo_packing_std140)); diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp index 5c0e4b67ff2..8e6455310bb 100644 --- a/src/compiler/glsl/linker.cpp +++ b/src/compiler/glsl/linker.cpp @@ -205,7 +205,8 @@ public: /* Generate a link error if the shader has declared this array with an * incorrect size. */ - if (size && size != this->num_vertices) { + if (!var->data.implicit_sized_array && + size && size != this->num_vertices) { linker_error(this->prog, "size of array %s declared as %u, " "but number of input vertices is %u\n", var->name, size, this->num_vertices); @@ -1494,8 +1495,11 @@ public: virtual ir_visitor_status visit(ir_variable *var) { const glsl_type *type_without_array; + bool implicit_sized_array = var->data.implicit_sized_array; fixup_type(&var->type, var->data.max_array_access, - var->data.from_ssbo_unsized_array); + var->data.from_ssbo_unsized_array, + &implicit_sized_array); + var->data.implicit_sized_array = implicit_sized_array; type_without_array = var->type->without_array(); if (var->type->is_interface()) { if (interface_contains_unsized_arrays(var->type)) { @@ -1553,11 +1557,12 @@ private: * 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, - bool from_ssbo_unsized_array) + bool from_ssbo_unsized_array, bool *implicit_sized) { if (!from_ssbo_unsized_array && (*type)->is_unsized_array()) { *type = glsl_type::get_array_instance((*type)->fields.array, max_array_access + 1); + *implicit_sized = true; assert(*type != NULL); } } @@ -1606,15 +1611,17 @@ private: memcpy(fields, type->fields.structure, num_fields * sizeof(*fields)); for (unsigned i = 0; i < num_fields; i++) { + bool implicit_sized_array = fields[i].implicit_sized_array; /* 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); + true, &implicit_sized_array); else fixup_type(&fields[i].type, max_ifc_array_access[i], - false); + false, &implicit_sized_array); + fields[i].implicit_sized_array = implicit_sized_array; } glsl_interface_packing packing = (glsl_interface_packing) type->interface_packing; @@ -2168,14 +2175,6 @@ link_intrastage_shaders(void *mem_ctx, if (!prog->LinkStatus) return NULL; - /* Link up uniform blocks defined within this stage. */ - link_uniform_blocks(mem_ctx, ctx, prog, shader_list, num_shaders, - &ubo_blocks, &num_ubo_blocks, &ssbo_blocks, - &num_ssbo_blocks); - - if (!prog->LinkStatus) - return NULL; - /* Check that there is only a single definition of each function signature * across all shaders. */ @@ -2239,24 +2238,6 @@ link_intrastage_shaders(void *mem_ctx, linked->ir = new(linked) exec_list; clone_ir_list(mem_ctx, linked->ir, main->ir); - /* Copy ubo blocks to linked shader list */ - linked->UniformBlocks = - ralloc_array(linked, gl_uniform_block *, num_ubo_blocks); - ralloc_steal(linked, ubo_blocks); - for (unsigned i = 0; i < num_ubo_blocks; i++) { - linked->UniformBlocks[i] = &ubo_blocks[i]; - } - linked->NumUniformBlocks = num_ubo_blocks; - - /* Copy ssbo blocks to linked shader list */ - linked->ShaderStorageBlocks = - ralloc_array(linked, gl_uniform_block *, num_ssbo_blocks); - ralloc_steal(linked, ssbo_blocks); - for (unsigned i = 0; i < num_ssbo_blocks; i++) { - linked->ShaderStorageBlocks[i] = &ssbo_blocks[i]; - } - linked->NumShaderStorageBlocks = num_ssbo_blocks; - link_fs_input_layout_qualifiers(prog, linked, shader_list, num_shaders); link_tcs_out_layout_qualifiers(prog, linked, shader_list, num_shaders); link_tes_in_layout_qualifiers(prog, linked, shader_list, num_shaders); @@ -2328,6 +2309,42 @@ link_intrastage_shaders(void *mem_ctx, return NULL; } + /* Make a pass over all variable declarations to ensure that arrays with + * unspecified sizes have a size specified. The size is inferred from the + * max_array_access field. + */ + array_sizing_visitor v; + v.run(linked->ir); + v.fixup_unnamed_interface_types(); + + /* Link up uniform blocks defined within this stage. */ + link_uniform_blocks(mem_ctx, ctx, prog, shader_list, num_shaders, + &ubo_blocks, &num_ubo_blocks, &ssbo_blocks, + &num_ssbo_blocks); + + if (!prog->LinkStatus) { + _mesa_delete_shader(ctx, linked); + return NULL; + } + + /* Copy ubo blocks to linked shader list */ + linked->UniformBlocks = + ralloc_array(linked, gl_uniform_block *, num_ubo_blocks); + ralloc_steal(linked, ubo_blocks); + for (unsigned i = 0; i < num_ubo_blocks; i++) { + linked->UniformBlocks[i] = &ubo_blocks[i]; + } + linked->NumUniformBlocks = num_ubo_blocks; + + /* Copy ssbo blocks to linked shader list */ + linked->ShaderStorageBlocks = + ralloc_array(linked, gl_uniform_block *, num_ssbo_blocks); + ralloc_steal(linked, ssbo_blocks); + for (unsigned i = 0; i < num_ssbo_blocks; i++) { + linked->ShaderStorageBlocks[i] = &ssbo_blocks[i]; + } + linked->NumShaderStorageBlocks = num_ssbo_blocks; + /* At this point linked should contain all of the linked IR, so * validate it to make sure nothing went wrong. */ @@ -2353,14 +2370,6 @@ link_intrastage_shaders(void *mem_ctx, } } - /* Make a pass over all variable declarations to ensure that arrays with - * unspecified sizes have a size specified. The size is inferred from the - * max_array_access field. - */ - array_sizing_visitor v; - v.run(linked->ir); - v.fixup_unnamed_interface_types(); - return linked; } diff --git a/src/compiler/glsl_types.h b/src/compiler/glsl_types.h index 7f9e3184ca9..f9a377d764c 100644 --- a/src/compiler/glsl_types.h +++ b/src/compiler/glsl_types.h @@ -916,6 +916,7 @@ struct glsl_struct_field { */ unsigned explicit_xfb_buffer:1; + unsigned implicit_sized_array:1; #ifdef __cplusplus glsl_struct_field(const struct glsl_type *_type, const char *_name) : type(_type), name(_name), location(-1), interpolation(0), centroid(0), -- 2.30.2