[Mesa-dev] [PATCH] glsl: handle implicit sized arrays in ssbo
Samuel Iglesias Gonsálvez
siglesias at igalia.com
Wed May 25 13:20:49 UTC 2016
On Wed, 2016-05-25 at 17:39 +1000, Dave Airlie wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> 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.
>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> ---
> 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 | 17 ++++--
> src/compiler/glsl/linker.cpp | 87 +++++++++++++++++--
> ------------
> src/compiler/glsl_types.h | 1 +
> 6 files changed, 67 insertions(+), 89 deletions(-)
>
> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> index 1ddb2bf..f025c1a 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 d52dbf8..e8efd27 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 9d05e7e..757f17c 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 ac415b5..3cb1a68 100644
> --- a/src/compiler/glsl/link_uniform_blocks.cpp
> +++ b/src/compiler/glsl/link_uniform_blocks.cpp
> @@ -34,9 +34,11 @@ 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)
indentation.
Assuming no dEQP nor piglit regressions,
Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
Sam
> : 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 +58,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 +151,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 +323,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 5c0e4b6..8e64553 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 7f9e318..f9a377d 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),
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160525/1883c92f/attachment-0001.sig>
More information about the mesa-dev
mailing list