[Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Ilia Mirkin
imirkin at alum.mit.edu
Thu Feb 11 16:39:14 UTC 2016
On Thu, Feb 11, 2016 at 11:31 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 | 79 ++++++++++++++++++++++++++++++++-----
> src/compiler/glsl/linker.cpp | 78 +++++++++++++++++++++++++-----------
> src/compiler/glsl/linker.h | 17 +++++++-
> src/mesa/main/mtypes.h | 8 ++++
> 4 files changed, 147 insertions(+), 35 deletions(-)
>
> diff --git a/src/compiler/glsl/link_uniforms.cpp b/src/compiler/glsl/link_uniforms.cpp
> index 7072c16..8192c21 100644
> --- a/src/compiler/glsl/link_uniforms.cpp
> +++ b/src/compiler/glsl/link_uniforms.cpp
> @@ -1038,9 +1038,36 @@ 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);
> +
> + list_for_each_entry_safe(struct empty_uniform_block, block,
> + &prog->EmptyUniformLocations, link) {
> + /* Found a block with enough slots to fit the uniform */
> + if (block->slots == entries) {
I haven't fully absorbed this patch, but if you have a bunch of holes
of 2 slots and then you try to allocate something needing 1 slot, then
you'll never decide you have a free block right? I think this needs to
be
block->slots >= entries
and then
if (block->slots > entries) {
block->slots -= entries;
block->start += entries;
} else {
list_del(&block->link);
ralloc_free(block);
}
Or am I misunderstanding something? This sort of greedy approach
doesn't guarantee optimal packing, you could do something much fancier
involving dynamic programming, but that hardly seems necessary.
> + unsigned start = block->start;
> + list_del(&block->link);
> + ralloc_free(block);
> +
> + 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 +1158,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 +1224,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 +1336,12 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>
> link_set_uniform_initializers(prog, boolean_true);
>
> + list_for_each_entry_safe(struct empty_uniform_block, block,
> + &prog->EmptyUniformLocations, link) {
> + ralloc_free(block);
> + }
> +
> + list_inithead(&prog->EmptyUniformLocations);
> +
> return;
> }
> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> index bad1c17..cbbb6f1 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,55 @@ 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;
> +
> + list_inithead(&prog->EmptyUniformLocations);
> +
> + struct empty_uniform_block *current_block = NULL;
> + unsigned prev_location = -1;
> +
> + 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 new continous block of empty slots */
> + if ((i == 0) || !current_block || (i != prev_location + 1)) {
> + current_block = ralloc(NULL, struct empty_uniform_block);
> + current_block->start = i;
> + current_block->slots = 1;
> + list_addtail(¤t_block->link, &prog->EmptyUniformLocations);
> + prev_location = i;
> + continue;
> + }
> +
> + /* The current block continues, so we simply increment its slots */
> + current_block->slots++;
> + prev_location = i;
> + }
> }
> +
> delete uniform_map;
> + return entries_total;
> }
>
> static bool
> @@ -4129,6 +4156,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
>
> @@ -4310,7 +4338,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 +4569,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..9f1fa3e 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 list_head 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..fcc9017 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 "util/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 list_head 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