[Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Manolova, Plamena
plamena.manolova at intel.com
Thu Feb 11 17:59:16 UTC 2016
Thank you for the review. You're absolutely right, I'll go ahead fix this.
On Thu, Feb 11, 2016 at 6:39 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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
> ---------------------------------------------------------------------
> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160211/6409abb5/attachment-0001.html>
More information about the mesa-dev
mailing list