[Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Manolova, Plamena
plamena.manolova at intel.com
Wed Feb 17 11:29:13 UTC 2016
On Wed, Feb 17, 2016 at 1:22 PM, Manolova, Plamena <
plamena.manolova at intel.com> wrote:
>
>
> On Mon, Feb 15, 2016 at 10:50 PM, Ilia Mirkin <imirkin at alum.mit.edu>
> wrote:
>
>> 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).
>
>
Thanks for spotting this, it must've slipped by me.
>
>> 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?
>
>
According to the spec:
https://www.opengl.org/registry/specs/ARB/explicit_uniform_location.txt:
"The explicitly defined locations
and the generated locations must be in the range of 0 to
MAX_UNIFORM_LOCATIONS minus one.", which means that the uniforms must fit
between array slots 0 - 98303, however if we use total_entries without
subtracting 1 we will be comparing to the range 1 - 98304 which will
trigger an error.
>
> > +
>> > + 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...
>>
>
Good point I'll fix this so it uses prog as the context.
>
>> > + 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
>> ---------------------------------------------------------------------
>> 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/20160217/d698faca/attachment-0001.html>
More information about the mesa-dev
mailing list