[Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Ilia Mirkin
imirkin at alum.mit.edu
Mon Feb 15 20:50:16 UTC 2016
In a few places your indentation is off -- please look at the 'git
diff' output, it should be pretty obvious. You used 2 spaces instead
of 3 (in just a handful of places).
On Fri, Feb 12, 2016 at 7:38 AM, Plamena Manolova
<plamena.manolova at intel.com> wrote:
> This patch moves the calculation of current uniforms to
> link_uniforms, which makes use of UniformRemapTable which
> stores all the reserved uniform locations.
>
> Location assignment for implicit uniforms now tries to use
> any gaps left in the table after the location assignment
> for explicit uniforms. This gives us more space to store more
> uniforms.
>
> Patch is based on earlier patch with following changes/additions:
>
> 1: Move the counting of explicit locations to
> check_explicit_uniform_locations and then pass
> the number to link_assign_uniform_locations.
> 2: Count the number of empty slots in UniformRemapTable
> and store them in a list_head.
> 3: Try to find an empty slot for implicit locations from
> the list, if that fails resize UniformRemapTable.
>
> Fixes following CTS tests:
> ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max
> ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max-array
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> Signed-off-by: Plamena Manolova <plamena.manolova at intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93696
> ---
> src/compiler/glsl/link_uniforms.cpp | 85 ++++++++++++++++++++++++++++++++-----
> src/compiler/glsl/linker.cpp | 73 ++++++++++++++++++++-----------
> src/compiler/glsl/linker.h | 17 +++++++-
> src/mesa/main/mtypes.h | 8 ++++
> 4 files changed, 148 insertions(+), 35 deletions(-)
>
> diff --git a/src/compiler/glsl/link_uniforms.cpp b/src/compiler/glsl/link_uniforms.cpp
> index 7072c16..aa07de3 100644
> --- a/src/compiler/glsl/link_uniforms.cpp
> +++ b/src/compiler/glsl/link_uniforms.cpp
> @@ -1038,9 +1038,43 @@ assign_hidden_uniform_slot_id(const char *name, unsigned hidden_id,
> uniform_size->map->put(hidden_uniform_start + hidden_id, name);
> }
>
> +/**
> + * Search through the list of empty blocks to find one that fits the current
> + * uniform.
> + */
> +static int
> +find_empty_block(struct gl_shader_program *prog,
> + struct gl_uniform_storage *uniform)
> +{
> + const unsigned entries = MAX2(1, uniform->array_elements);
> +
> + foreach_list_typed(struct empty_uniform_block, block, link,
> + &prog->EmptyUniformLocations) {
> + /* Found a block with enough slots to fit the uniform */
> + if (block->slots == entries) {
> + unsigned start = block->start;
> + exec_node_remove(&block->link);
> + ralloc_free(block);
> +
> + return start;
> + /* Found a block with more slots than needed. It can still be used. */
> + } else if (block->slots > entries) {
> + unsigned start = block->start;
> + block->start += entries;
> + block->slots -= entries;
> +
> + return start;
> + }
> + }
> +
> + return -1;
> +}
> +
> void
> link_assign_uniform_locations(struct gl_shader_program *prog,
> - unsigned int boolean_true)
> + unsigned int boolean_true,
> + unsigned int num_explicit_uniform_locs,
> + unsigned int max_uniform_locs)
> {
> ralloc_free(prog->UniformStorage);
> prog->UniformStorage = NULL;
> @@ -1131,6 +1165,9 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>
> parcel_out_uniform_storage parcel(prog, prog->UniformHash, uniforms, data);
>
> + unsigned total_entries = num_explicit_uniform_locs;
> + unsigned empty_locs = prog->NumUniformRemapTable - num_explicit_uniform_locs;
> +
> for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> if (prog->_LinkedShaders[i] == NULL)
> continue;
> @@ -1194,21 +1231,43 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
> /* how many new entries for this uniform? */
> const unsigned entries = MAX2(1, uniforms[i].array_elements);
>
> - /* resize remap table to fit new entries */
> - prog->UniformRemapTable =
> - reralloc(prog,
> - prog->UniformRemapTable,
> - gl_uniform_storage *,
> - prog->NumUniformRemapTable + entries);
> + /* Find UniformRemapTable for empty blocks where we can fit this uniform. */
> + int chosen_location = -1;
> +
> + if (empty_locs)
> + chosen_location = find_empty_block(prog, &uniforms[i]);
> +
> + if (chosen_location != -1) {
> + empty_locs -= entries;
> + } else {
> + chosen_location = prog->NumUniformRemapTable;
> +
> + /* Add new entries to the total amount of entries. */
> + total_entries += entries;
> +
> + /* resize remap table to fit new entries */
> + prog->UniformRemapTable =
> + reralloc(prog,
> + prog->UniformRemapTable,
> + gl_uniform_storage *,
> + prog->NumUniformRemapTable + entries);
> + prog->NumUniformRemapTable += entries;
> + }
>
> /* set pointers for this uniform */
> for (unsigned j = 0; j < entries; j++)
> - prog->UniformRemapTable[prog->NumUniformRemapTable+j] = &uniforms[i];
> + prog->UniformRemapTable[chosen_location + j] = &uniforms[i];
>
> /* set the base location in remap table for the uniform */
> - uniforms[i].remap_location = prog->NumUniformRemapTable;
> + uniforms[i].remap_location = chosen_location;
> + }
>
> - prog->NumUniformRemapTable += entries;
> + /* Verify that total amount of entries for explicit and implicit locations
> + * is less than MAX_UNIFORM_LOCATIONS.
> + */
> + if (total_entries >= max_uniform_locs) {
> + linker_error(prog, "count of uniform locations >= MAX_UNIFORM_LOCATIONS"
> + "(%u >= %u)", total_entries, max_uniform_locs);
> }
>
> /* Reserve all the explicit locations of the active subroutine uniforms. */
> @@ -1284,5 +1343,11 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>
> link_set_uniform_initializers(prog, boolean_true);
>
> + foreach_list_typed(struct empty_uniform_block, block, link,
> + &prog->EmptyUniformLocations) {
> + ralloc_free(block);
> + }
> + exec_list_make_empty(&prog->EmptyUniformLocations);
> +
> return;
> }
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index bad1c17..ce4f07c 100644
> --- a/src/compiler/glsl/linker.cpp
> +++ b/src/compiler/glsl/linker.cpp
> @@ -3008,12 +3008,13 @@ check_image_resources(struct gl_context *ctx, struct gl_shader_program *prog)
> * for a variable, checks for overlaps between other uniforms using explicit
> * locations.
> */
> -static bool
> +static int
> reserve_explicit_locations(struct gl_shader_program *prog,
> string_to_uint_map *map, ir_variable *var)
> {
> unsigned slots = var->type->uniform_locations();
> unsigned max_loc = var->data.location + slots - 1;
> + unsigned return_value = slots;
>
> /* Resize remap table if locations do not fit in the current one. */
> if (max_loc + 1 > prog->NumUniformRemapTable) {
> @@ -3024,7 +3025,7 @@ reserve_explicit_locations(struct gl_shader_program *prog,
>
> if (!prog->UniformRemapTable) {
> linker_error(prog, "Out of memory during linking.\n");
> - return false;
> + return -1;
> }
>
> /* Initialize allocated space. */
> @@ -3042,8 +3043,10 @@ reserve_explicit_locations(struct gl_shader_program *prog,
>
> /* Possibly same uniform from a different stage, this is ok. */
> unsigned hash_loc;
> - if (map->get(hash_loc, var->name) && hash_loc == loc - i)
> - continue;
> + if (map->get(hash_loc, var->name) && hash_loc == loc - i) {
> + return_value = 0;
> + continue;
> + }
>
> /* ARB_explicit_uniform_location specification states:
> *
> @@ -3055,7 +3058,7 @@ reserve_explicit_locations(struct gl_shader_program *prog,
> "location qualifier for uniform %s overlaps "
> "previously used location\n",
> var->name);
> - return false;
> + return -1;
> }
>
> /* Initialize location as inactive before optimization
> @@ -3067,7 +3070,7 @@ reserve_explicit_locations(struct gl_shader_program *prog,
> /* Note, base location used for arrays. */
> map->put(var->data.location, var->name);
>
> - return true;
> + return return_value;
> }
>
> static bool
> @@ -3128,12 +3131,12 @@ reserve_subroutine_explicit_locations(struct gl_shader_program *prog,
> * any optimizations happen to handle also inactive uniforms and
> * inactive array elements that may get trimmed away.
> */
> -static void
> +static int
> check_explicit_uniform_locations(struct gl_context *ctx,
> struct gl_shader_program *prog)
> {
> if (!ctx->Extensions.ARB_explicit_uniform_location)
> - return;
> + return -1;
>
> /* This map is used to detect if overlapping explicit locations
> * occur with the same uniform (from different stage) or a different one.
> @@ -3142,7 +3145,7 @@ check_explicit_uniform_locations(struct gl_context *ctx,
>
> if (!uniform_map) {
> linker_error(prog, "Out of memory during linking.\n");
> - return;
> + return -1;
> }
>
> unsigned entries_total = 0;
> @@ -3157,31 +3160,50 @@ check_explicit_uniform_locations(struct gl_context *ctx,
> if (!var || var->data.mode != ir_var_uniform)
> continue;
>
> - entries_total += var->type->uniform_locations();
> -
> if (var->data.explicit_location) {
> - bool ret;
> + bool ret = false;
> if (var->type->without_array()->is_subroutine())
> ret = reserve_subroutine_explicit_locations(prog, sh, var);
> - else
> - ret = reserve_explicit_locations(prog, uniform_map, var);
> + else {
> + int slots = reserve_explicit_locations(prog, uniform_map,
> + var);
> + if (slots != -1) {
> + ret = true;
> + entries_total += slots;
> + }
> + }
> if (!ret) {
> delete uniform_map;
> - return;
> + return -1;
> }
> }
> }
> }
>
> - /* Verify that total amount of entries for explicit and implicit locations
> - * is less than MAX_UNIFORM_LOCATIONS.
> - */
> - if (entries_total >= ctx->Const.MaxUserAssignableUniformLocations) {
> - linker_error(prog, "count of uniform locations >= MAX_UNIFORM_LOCATIONS"
> - "(%u >= %u)", entries_total,
> - ctx->Const.MaxUserAssignableUniformLocations);
> + if (entries_total > 0)
> + entries_total -= 1;
This seems weird... why are you doing that?
> +
> + exec_list_make_empty(&prog->EmptyUniformLocations);
> + struct empty_uniform_block *current_block = NULL;
> +
> + for (unsigned i = 0; i < prog->NumUniformRemapTable; i++) {
> + /* We found empty space in UniformRemapTable. */
> + if (prog->UniformRemapTable[i] == NULL) {
> + /* We've found the beginning of a new continous block of empty slots */
> + if (!current_block || current_block->start + current_block->slots != i) {
> + current_block = rzalloc(NULL, struct empty_uniform_block);
is using ralloc with a NULL context a good idea? I don't know too much
about it, but I thought the whole point of it was to have non-null
contexts... should you be using an existing ralloc context, or just
use calloc? Not sure what the policy is...
> + current_block->start = i;
> + exec_list_push_tail(&prog->EmptyUniformLocations,
> + ¤t_block->link);
> + }
> +
> + /* The current block continues, so we simply increment its slots */
> + current_block->slots++;
> + }
> }
> +
> delete uniform_map;
> + return entries_total;
> }
>
> static bool
> @@ -4129,6 +4151,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
>
> tfeedback_decl *tfeedback_decls = NULL;
> unsigned num_tfeedback_decls = prog->TransformFeedback.NumVarying;
> + unsigned int num_explicit_uniform_locs = 0;
>
> void *mem_ctx = ralloc_context(NULL); // temporary linker context
This is the ralloc context you should use... and then you don't have
to worry about freeing stuff "by hand". Just drop it and it'll get
magically cleaned up at the end.
>
> @@ -4310,7 +4333,7 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> last = i;
> }
>
> - check_explicit_uniform_locations(ctx, prog);
> + num_explicit_uniform_locs = check_explicit_uniform_locations(ctx, prog);
> link_assign_subroutine_types(prog);
>
> if (!prog->LinkStatus)
> @@ -4541,7 +4564,9 @@ link_shaders(struct gl_context *ctx, struct gl_shader_program *prog)
> goto done;
>
> update_array_sizes(prog);
> - link_assign_uniform_locations(prog, ctx->Const.UniformBooleanTrue);
> + link_assign_uniform_locations(prog, ctx->Const.UniformBooleanTrue,
> + num_explicit_uniform_locs,
> + ctx->Const.MaxUserAssignableUniformLocations);
> link_assign_atomic_counter_resources(ctx, prog);
> store_fragdepth_layout(prog);
>
> diff --git a/src/compiler/glsl/linker.h b/src/compiler/glsl/linker.h
> index c80be1c..a60bb6e 100644
> --- a/src/compiler/glsl/linker.h
> +++ b/src/compiler/glsl/linker.h
> @@ -35,7 +35,9 @@ link_invalidate_variable_locations(exec_list *ir);
>
> extern void
> link_assign_uniform_locations(struct gl_shader_program *prog,
> - unsigned int boolean_true);
> + unsigned int boolean_true,
> + unsigned int num_explicit_uniform_locs,
> + unsigned int max_uniform_locs);
>
> extern void
> link_set_uniform_initializers(struct gl_shader_program *prog,
> @@ -202,4 +204,17 @@ linker_error(gl_shader_program *prog, const char *fmt, ...);
> void
> linker_warning(gl_shader_program *prog, const char *fmt, ...);
>
> +/**
> + * Sometimes there are empty slots left over in UniformRemapTable after we
> + * allocate slots to explicit locations. This struct represents a single
> + * continouous block of empty slots in UniformRemapTable.
> + */
> +struct empty_uniform_block {
> + struct exec_node link;
> + /* The start location of the block */
> + unsigned start;
> + /* The number of slots in the block */
> + unsigned slots;
> +};
> +
> #endif /* GLSL_LINKER_H */
> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
> index e987177..aeb8043 100644
> --- a/src/mesa/main/mtypes.h
> +++ b/src/mesa/main/mtypes.h
> @@ -44,6 +44,7 @@
> #include "math/m_matrix.h" /* GLmatrix */
> #include "compiler/shader_enums.h"
> #include "main/formats.h" /* MESA_FORMAT_COUNT */
> +#include "compiler/glsl/list.h"
>
>
> #ifdef __cplusplus
> @@ -2764,6 +2765,13 @@ struct gl_shader_program
> struct gl_uniform_storage **UniformRemapTable;
>
> /**
> + * Sometimes there are empty slots left over in UniformRemapTable after we
> + * allocate slots to explicit locations. This list stores the blocks of
> + * continuous empty slots inside UniformRemapTable.
> + */
> + struct exec_list EmptyUniformLocations;
> +
> + /**
> * Size of the gl_ClipDistance array that is output from the last pipeline
> * stage before the fragment shader.
> */
> --
> 2.4.3
>
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list