I have minor comments below. I don't see anything wrong here, but maybe
others with more knowledge of this area want to review this patch as well.
With the minor comments addressed,
Reviewed-by: Samuel Iglesias Gonsálvez <sigles...@igalia.com>
On 07/10/15 00:47, Timothy Arceri wrote:
> This adds support for setting up the UniformBlock structures for AoA
> and also adds support for resizing AoA blocks with a packed layout.
> ---
> src/glsl/link_uniform_block_active_visitor.cpp | 168
> ++++++++++++++++---------
> src/glsl/link_uniform_block_active_visitor.h | 13 +-
> src/glsl/link_uniform_blocks.cpp | 160 +++++++++++++++--------
> 3 files changed, 229 insertions(+), 112 deletions(-)
>
> diff --git a/src/glsl/link_uniform_block_active_visitor.cpp
> b/src/glsl/link_uniform_block_active_visitor.cpp
> index bcf17fe..422739a 100644
> --- a/src/glsl/link_uniform_block_active_visitor.cpp
> +++ b/src/glsl/link_uniform_block_active_visitor.cpp
> @@ -71,6 +71,88 @@ process_block(void *mem_ctx, struct hash_table *ht,
> ir_variable *var)
> return NULL;
> }
>
> +/* For arrays of arrays this function will give us a middle ground between
> + * detecting inactive uniform blocks and structuring them in a way that makes
> + * it easy to calculate the offset for indirect indexing.
> + *
> + * For example given the shader:
> + *
> + * uniform ArraysOfArraysBlock
> + * {
> + * vec4 a;
> + * } i[3][4][5];
> + *
> + * void main()
> + * {
> + * vec4 b = i[0][1][1].a;
> + * gl_Position = i[2][2][3].a + b;
> + * }
> + *
> + * There are only 2 active blocks above but for the sake of indirect indexing
> + * and not over complicating the code we will end up with a count of 8.
> + * Here each dimension has 2 different indices counted so we end up with
> 2*2*2
> + */
> +struct uniform_block_array_elements **
> +process_arrays(void *mem_ctx, ir_dereference_array *ir,
> + struct link_uniform_block_active *block)
> +{
> + if (ir) {
> + struct uniform_block_array_elements **ub_array_ptr =
> + process_arrays(mem_ctx, ir->array->as_dereference_array(), block);
> + if (*ub_array_ptr == NULL) {
> + *ub_array_ptr = rzalloc(mem_ctx, struct
> uniform_block_array_elements);
> + (*ub_array_ptr)->ir = ir;
> + }
> +
> + struct uniform_block_array_elements *ub_array = *ub_array_ptr;
> + ir_constant *c = ir->array_index->as_constant();
> + if (c) {
> + /* Index is a constant, so mark just that element used,
> + * if not already.
> + */
> + const unsigned idx = c->get_uint_component(0);
> +
> + unsigned i;
> + for (i = 0; i < ub_array->num_array_elements; i++) {
> + if (ub_array->array_elements[i] == idx)
> + break;
> + }
> +
> + assert(i <= ub_array->num_array_elements);
> +
> + if (i == ub_array->num_array_elements) {
> + ub_array->array_elements = reralloc(mem_ctx,
> + ub_array->array_elements,
> + unsigned,
> + ub_array->num_array_elements
> + 1);
> +
> + ub_array->array_elements[ub_array->num_array_elements] = idx;
> +
> + ub_array->num_array_elements++;
> + }
> + } else {
> + /* The array index is not a constant,
> + * so mark the entire array used.
> + */
> + assert(ir->array->type->is_array());
> + if (ub_array->num_array_elements < ir->array->type->length) {
> + ub_array->num_array_elements = ir->array->type->length;
> + ub_array->array_elements = reralloc(mem_ctx,
> + ub_array->array_elements,
> + unsigned,
> +
> ub_array->num_array_elements);
> +
> + for (unsigned i = 0; i < ub_array->num_array_elements; i++) {
> + ub_array->array_elements[i] = i;
> + }
> + }
> + }
> + return &ub_array->array;
> + } else {
> + return &block->array;
> + }
> +}
> +
> ir_visitor_status
> link_uniform_block_active_visitor::visit(ir_variable *var)
> {
> @@ -101,24 +183,30 @@ link_uniform_block_active_visitor::visit(ir_variable
> *var)
> return visit_stop;
> }
>
> - assert(b->num_array_elements == 0);
> - assert(b->array_elements == NULL);
> + assert(b->array == NULL);
> assert(b->type != NULL);
> assert(!b->type->is_array() || b->has_instance_name);
>
> /* For uniform block arrays declared with a shared or std140 layout
> * qualifier, mark all its instances as used.
> */
> - if (b->type->is_array() && b->type->length > 0) {
> - b->num_array_elements = b->type->length;
> - b->array_elements = reralloc(this->mem_ctx,
> - b->array_elements,
> - unsigned,
> - b->num_array_elements);
> -
> - for (unsigned i = 0; i < b->num_array_elements; i++) {
> - b->array_elements[i] = i;
> + const glsl_type *type = b->type;
> + struct uniform_block_array_elements **ub_array = &b->array;
> + while (type->is_array()) {
> + assert(b->type->length > 0);
> +
assert(type->length > 0);
> + *ub_array = rzalloc(this->mem_ctx, struct
> uniform_block_array_elements);
> + (*ub_array)->num_array_elements = type->length;
> + (*ub_array)->array_elements = reralloc(this->mem_ctx,
> + (*ub_array)->array_elements,
> + unsigned,
> +
> (*ub_array)->num_array_elements);
> +
> + for (unsigned i = 0; i < (*ub_array)->num_array_elements; i++) {
> + (*ub_array)->array_elements[i] = i;
> }
> + ub_array = &(*ub_array)->array;
> + type = type->fields.array;
> }
>
> return visit_continue;
> @@ -127,7 +215,13 @@ link_uniform_block_active_visitor::visit(ir_variable
> *var)
> ir_visitor_status
> link_uniform_block_active_visitor::visit_enter(ir_dereference_array *ir)
> {
> - ir_dereference_variable *const d = ir->array->as_dereference_variable();
> + /* cycle through arrays of arrays */
> + ir_dereference_array *base_ir = ir;
> + while (base_ir->array->ir_type == ir_type_dereference_array)
> + base_ir = base_ir->array->as_dereference_array();
> +
> + ir_dereference_variable *const d =
> + base_ir->array->as_dereference_variable();
> ir_variable *const var = (d == NULL) ? NULL : d->var;
>
> /* If the r-value being dereferenced is not a variable (e.g., a field of a
> @@ -158,55 +252,16 @@
> link_uniform_block_active_visitor::visit_enter(ir_dereference_array *ir)
> /* Block arrays must be declared with an instance name.
> */
> assert(b->has_instance_name);
> - assert((b->num_array_elements == 0) == (b->array_elements == NULL));
> assert(b->type != NULL);
>
> /* If the block array was declared with a shared or
> * std140 layout qualifier, all its instances have been already marked
> * as used in link_uniform_block_active_visitor::visit(ir_variable *).
> */
> - if (var->get_interface_type()->interface_packing !=
> - GLSL_INTERFACE_PACKING_PACKED)
> - return visit_continue_with_parent;
> -
> - ir_constant *c = ir->array_index->as_constant();
> -
> - if (c) {
> - /* Index is a constant, so mark just that element used, if not already
> */
> - const unsigned idx = c->get_uint_component(0);
> -
> - unsigned i;
> - for (i = 0; i < b->num_array_elements; i++) {
> - if (b->array_elements[i] == idx)
> - break;
> - }
> -
> - assert(i <= b->num_array_elements);
> -
> - if (i == b->num_array_elements) {
> - b->array_elements = reralloc(this->mem_ctx,
> - b->array_elements,
> - unsigned,
> - b->num_array_elements + 1);
> -
> - b->array_elements[b->num_array_elements] = idx;
> -
> - b->num_array_elements++;
> - }
> - } else {
> - /* The array index is not a constant, so mark the entire array used. */
> - assert(b->type->is_array());
> - if (b->num_array_elements < b->type->length) {
> - b->num_array_elements = b->type->length;
> - b->array_elements = reralloc(this->mem_ctx,
> - b->array_elements,
> - unsigned,
> - b->num_array_elements);
> -
> - for (unsigned i = 0; i < b->num_array_elements; i++) {
> - b->array_elements[i] = i;
> - }
> - }
> + if (var->get_interface_type()->interface_packing ==
> + GLSL_INTERFACE_PACKING_PACKED) {
> + b->var = var;
> + process_arrays(this->mem_ctx, ir, b);
> }
>
> return visit_continue_with_parent;
> @@ -234,8 +289,7 @@
> link_uniform_block_active_visitor::visit(ir_dereference_variable *ir)
> return visit_stop;
> }
>
> - assert(b->num_array_elements == 0);
> - assert(b->array_elements == NULL);
> + assert(b->array == NULL);
> assert(b->type != NULL);
>
> return visit_continue;
> diff --git a/src/glsl/link_uniform_block_active_visitor.h
> b/src/glsl/link_uniform_block_active_visitor.h
> index b663a88..afb52c1 100644
> --- a/src/glsl/link_uniform_block_active_visitor.h
> +++ b/src/glsl/link_uniform_block_active_visitor.h
> @@ -28,11 +28,20 @@
> #include "ir.h"
> #include "util/hash_table.h"
>
> +struct uniform_block_array_elements {
> + unsigned *array_elements;
> + unsigned num_array_elements;
> +
> + ir_dereference_array *ir;
> +
> + struct uniform_block_array_elements *array;
> +};
> +
> struct link_uniform_block_active {
> const glsl_type *type;
> + ir_variable *var;
>
> - unsigned *array_elements;
> - unsigned num_array_elements;
> + struct uniform_block_array_elements *array;
>
> unsigned binding;
>
> diff --git a/src/glsl/link_uniform_blocks.cpp
> b/src/glsl/link_uniform_blocks.cpp
> index 7ceffee..5285d8d 100644
> --- a/src/glsl/link_uniform_blocks.cpp
> +++ b/src/glsl/link_uniform_blocks.cpp
> @@ -116,7 +116,7 @@ private:
> char *open_bracket = strchr(v->IndexName, '[');
> assert(open_bracket != NULL);
>
> - char *close_bracket = strchr(open_bracket, ']');
> + char *close_bracket = strchr(open_bracket, '.') - 1;
> assert(close_bracket != NULL);
>
I would assert *close_bracket is ']'.
> /* Length of the tail without the ']' but with the NUL.
> @@ -185,6 +185,91 @@ struct block {
> bool has_instance_name;
> };
>
> +static void
> +process_block_array(struct uniform_block_array_elements *ub_array, char
> **name,
> + size_t name_length, gl_uniform_block *blocks,
> + ubo_visitor *parcel, gl_uniform_buffer_variable
> *variables,
> + const struct link_uniform_block_active *const b,
> + unsigned *block_index, unsigned *binding_offset,
> + struct gl_context *ctx, struct gl_shader_program *prog)
> +{
> + if (ub_array) {
> + for (unsigned j = 0; j < ub_array->num_array_elements; j++) {
> + size_t new_length = name_length;
> +
Indentation
> + /* Append the subscript to the current variable name */
> + ralloc_asprintf_rewrite_tail(name, &new_length, "[%u]",
> + ub_array->array_elements[j]);
> +
> + process_block_array(ub_array->array, name, new_length, blocks,
> + parcel, variables, b, block_index,
> + binding_offset, ctx, prog);
> + }
> + } else {
> + unsigned i = *block_index;
> + const glsl_type *type = b->type->without_array();
> +
> + blocks[i].Name = ralloc_strdup(blocks, *name);
> + blocks[i].Uniforms = &variables[(*parcel).index];
> +
> + /* The GL_ARB_shading_language_420pack spec says:
> + *
> + * "If the binding identifier is used with a uniform block
> + * instanced as an array then the first element of the array
> + * takes the specified block binding and each subsequent
> + * element takes the next consecutive uniform block binding
> + * point."
> + */
> + blocks[i].Binding = (b->has_binding) ? b->binding + *binding_offset :
> 0;
> +
> + blocks[i].UniformBufferSize = 0;
> + blocks[i]._Packing = gl_uniform_block_packing(type->interface_packing);
> +
> + parcel->process(type, blocks[i].Name);
> +
> + blocks[i].UniformBufferSize = parcel->buffer_size;
> +
> + /* Check SSBO size is lower than maximum supported size for SSBO */
> + if (b->is_shader_storage &&
> + parcel->buffer_size > ctx->Const.MaxShaderStorageBlockSize) {
> + linker_error(prog, "shader storage block `%s' has size %d, "
> + "which is larger than than the maximum allowed (%d)",
> + b->type->name,
> + parcel->buffer_size,
> + ctx->Const.MaxShaderStorageBlockSize);
> + }
> + blocks[i].NumUniforms =
> + (unsigned)(ptrdiff_t)(&variables[parcel->index] -
> blocks[i].Uniforms);
> + blocks[i].IsShaderStorage = b->is_shader_storage;
> +
> + *block_index = *block_index + 1;
> + *binding_offset = *binding_offset + 1;
> + }
> +}
> +
> +/* This function resizes the array types of the block so that later we can
> use
> + * this new size to correctly calculate the offest for indirect indexing.
s/offest/offset
> + */
> +const glsl_type *
> +resize_block_array(const glsl_type *type,
> + struct uniform_block_array_elements *ub_array)
This should be a static function, right?
> +{
> + if (type->is_array()) {
> + struct uniform_block_array_elements *child_array =
> + type->fields.array->is_array() ? ub_array->array : NULL;
> + const glsl_type *new_child_type =
> + resize_block_array(type->fields.array, child_array);
> +
> + const glsl_type *new_type =
> + glsl_type::get_array_instance(new_child_type,
> + ub_array->num_array_elements);
> + ub_array->ir->array->type = new_type;
> + return new_type;
> + } else {
> + return type;
> + }
> +}
> +
> unsigned
> link_uniform_blocks(void *mem_ctx,
> struct gl_context *ctx,
> @@ -223,21 +308,25 @@ link_uniform_blocks(void *mem_ctx,
> struct hash_entry *entry;
>
> hash_table_foreach (block_hash, entry) {
> - const struct link_uniform_block_active *const b =
> - (const struct link_uniform_block_active *) entry->data;
> + struct link_uniform_block_active *const b =
> + (struct link_uniform_block_active *) entry->data;
>
> - const glsl_type *const block_type =
> - b->type->is_array() ? b->type->fields.array : b->type;
> + assert((b->array != NULL) == b->type->is_array());
>
> - assert((b->num_array_elements > 0) == b->type->is_array());
> + if (b->array != NULL &&
> + (b->type->without_array()->interface_packing ==
> + GLSL_INTERFACE_PACKING_PACKED)) {
> + b->type = resize_block_array(b->type, b->array);
> + b->var->type = b->type;
> + }
>
> block_size.num_active_uniforms = 0;
> - block_size.process(block_type, "");
> + block_size.process(b->type->without_array(), "");
>
> - if (b->num_array_elements > 0) {
> - num_blocks += b->num_array_elements;
> - num_variables += b->num_array_elements
> - * block_size.num_active_uniforms;
> + if (b->array != NULL) {
> + unsigned aoa_size = b->type->arrays_of_arrays_size();
> + num_blocks += aoa_size;
> + num_variables += aoa_size * block_size.num_active_uniforms;
> } else {
> num_blocks++;
> num_variables += block_size.num_active_uniforms;
> @@ -281,50 +370,15 @@ link_uniform_blocks(void *mem_ctx,
> (const struct link_uniform_block_active *) entry->data;
> const glsl_type *block_type = b->type;
>
> - if (b->num_array_elements > 0) {
> - const char *const name = block_type->fields.array->name;
> + if (b->array != NULL) {
> + unsigned binding_offset = 0;
> + char *name = ralloc_strdup(NULL, block_type->without_array()->name);
> + size_t name_length = strlen(name);
>
> assert(b->has_instance_name);
> - for (unsigned j = 0; j < b->num_array_elements; j++) {
> - blocks[i].Name = ralloc_asprintf(blocks, "%s[%u]", name,
> - b->array_elements[j]);
> - blocks[i].Uniforms = &variables[parcel.index];
> -
> - /* The GL_ARB_shading_language_420pack spec says:
> - *
> - * "If the binding identifier is used with a uniform block
> - * instanced as an array then the first element of the array
> - * takes the specified block binding and each subsequent
> - * element takes the next consecutive uniform block binding
> - * point."
> - */
> - blocks[i].Binding = (b->has_binding) ? b->binding + j : 0;
> -
> - blocks[i].UniformBufferSize = 0;
> - blocks[i]._Packing =
> - gl_uniform_block_packing(block_type->interface_packing);
> -
> - parcel.process(block_type->fields.array,
> - blocks[i].Name);
> -
> - blocks[i].UniformBufferSize = parcel.buffer_size;
> -
> - /* Check SSBO size is lower than maximum supported size for SSBO
> */
> - if (b->is_shader_storage &&
> - parcel.buffer_size > ctx->Const.MaxShaderStorageBlockSize) {
> - linker_error(prog, "shader storage block `%s' has size %d, "
> - "which is larger than than the maximum allowed
> (%d)",
> - block_type->name,
> - parcel.buffer_size,
> - ctx->Const.MaxShaderStorageBlockSize);
> - }
> - blocks[i].NumUniforms =
> - (unsigned)(ptrdiff_t)(&variables[parcel.index] -
> blocks[i].Uniforms);
> -
> - blocks[i].IsShaderStorage = b->is_shader_storage;
> -
> - i++;
> - }
> + process_block_array(b->array, &name, name_length, blocks, &parcel,
> + variables, b, &i, &binding_offset, ctx, prog);
> + ralloc_free(name);
> } else {
> blocks[i].Name = ralloc_strdup(blocks, block_type->name);
> blocks[i].Uniforms = &variables[parcel.index];
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
