[Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Matt Turner
mattst88 at gmail.com
Mon Feb 15 21:38:00 UTC 2016
On Mon, Feb 15, 2016 at 12: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).
If you use vim, you can put something like this in your ~/.vimrc:
autocmd BufNewFile,BufRead /home/mattst88/projects/mesa/* set
expandtab tabstop=8 softtabstop=3 shiftwidth=3
autocmd BufNewFile,BufRead
/home/mattst88/projects/mesa/src/glsl/glcpp/* set noexpandtab
tabstop=8 softtabstop=8 shiftwidth=8
autocmd BufNewFile,BufRead
/home/mattst88/projects/mesa/src/glsl/glsl_parser.yy set noexpandtab
tabstop=8 shiftwidth=8
autocmd BufNewFile,BufRead /home/mattst88/projects/piglit/* set
noexpandtab tabstop=8 softtabstop=8 shiftwidth=8
autocmd BufNewFile,BufRead Makefile* set noexpandtab tabstop=8
softtabstop=8 shiftwidth=8
autocmd BufNewFile,BufRead *.mk set noexpandtab tabstop=8
softtabstop=8 shiftwidth=8
(it'll probably get line wrapped by gmail)
> 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...
Right -- if it's not using a context, there's not much point.
Either using the temporary mem_ctx, or just using malloc/free is the
right thing to do. I wouldn't feel bad about using malloc/free if you
know exactly where the object's lifetime ends.
More information about the mesa-dev
mailing list