[Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Connor Abbott
cwabbott0 at gmail.com
Mon Feb 15 22:02:32 UTC 2016
On Mon, Feb 15, 2016 at 4:38 PM, Matt Turner <mattst88 at gmail.com> wrote:
> 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)
or just use https://github.com/chadversary/vim-mesa that also has the
right cinoptions.
>
>> 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.
> _______________________________________________
> 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