[Mesa-dev] [PATCH v2] glsl: move uniform calculation to link_uniforms
Tapani Pälli
tapani.palli at intel.com
Thu Jan 21 23:45:47 PST 2016
On 01/20/2016 09:21 PM, Ian Romanick wrote:
> So... I'm just going to say that this is commit is the new poster child
> for not committing something as soon as you get a positive review. This
> patch went out barely 36 hours ago, and there is already a fix-up patch
> for it. I have also found two other issues in review.
>
> We're not in that much of a hurry. As one of my engineering professors
> used to say, "Fast is slow."
>
> On 01/19/2016 02:17 AM, Tapani Pälli wrote:
>> Patch moves uniform calculation to happen during link_uniforms, this
>> is possible with help of UniformRemapTable that has all the reserved
>> locations.
>>
>> Location assignment for implicit locations is changed so that we
>> utilize also the 'holes' that explicit uniform location assignment
>> might have left in UniformRemapTable, this makes it possible to fit
>> more uniforms as previously we were lazy here and wasting space.
>>
>> 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
>>
>> v2: code cleanups (Matt), increment NumUniformRemapTable correctly
>> (Timothy), fix find_empty_block to work like intended (sigh) and
>> add some more comments.
>>
>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>> ---
>> src/glsl/link_uniforms.cpp | 87 ++++++++++++++++++++++++++++++++++++++++------
>> src/glsl/linker.cpp | 19 ++++------
>> src/glsl/linker.h | 3 +-
>> 3 files changed, 85 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
>> index 33b2d4c..76ee70d 100644
>> --- a/src/glsl/link_uniforms.cpp
>> +++ b/src/glsl/link_uniforms.cpp
>> @@ -1057,9 +1057,40 @@ assign_hidden_uniform_slot_id(const char *name, unsigned hidden_id,
>> uniform_size->map->put(hidden_uniform_start + hidden_id, name);
>> }
>>
>> +/**
>> + * Search UniformRemapTable for empty block big enough to hold given uniform.
>> + * TODO Optimize this algorithm later if it turns out to be a major bottleneck.
>> + */
>> +static int
>> +find_empty_block(struct gl_shader_program *prog,
>> + struct gl_uniform_storage *uniform)
>> +{
>> + const unsigned entries = MAX2(1, uniform->array_elements);
>> + for (unsigned i = 0, j; i < prog->NumUniformRemapTable; i++) {
>
> Please put the declaration of j by itself. I know it can't go in the
> for-loop below, but I think we can spare the extra line of code to make
> things more readable.
ok
>> + /* We found empty space in UniformRemapTable. */
>> + if (prog->UniformRemapTable[i] == NULL) {
>> + for (j = i; j < entries && j < prog->NumUniformRemapTable; j++) {
>> + if (prog->UniformRemapTable[j] != NULL) {
>> + /* Entries do not fit in this space, continue searching
>> + * after this location.
>> + */
>> + i = j + 1;
>
> I'm pretty sure this should be 'i = j'. Before examining
> prog->UniformRemapTable[i] again, i will be incremented. As a result,
> the j+1 element will never be examined. If j+1 is the start of the only
> gap of entries elements, you'll miss it. Since this is a stand-alone
> function, it is a good candidate for some unit tests.
True, this was a last-minute addition and I did not pay enough attention
to it. We definitely need more tests for this functionality as the CTS
tests are quite simple since they always leave the 'hole' at the
beginning of the table.
>> + break;
>> + }
>> + }
>> + /* Entries fit, we can return this location. */
>> + if (i != j + 1) {
>
> After making the change above, this will also need to change. It may be
> possible to slightly change the inner loop logic. Maybe:
>
> /* Not enough space left in the table for the required number
> * of entries.
> */
> if (i + entries > prog->NumUniformRemapTable)
> return -1;
>
> unsigned gap;
> for (gap = 1; gap < entries; gap++) {
> if (prog->UniformRemapTable[i + gap] != NULL) {
> /* Entries do not fit in this space, continue searching
> * after this location. The outer loop increment
> * advances past the i+gap element that was already
> * examined.
> */
> gap = 0;
> i += gap;
> break;
> }
> }
>
> if (gap != 0)
> return i;
Yeah, I originally had extra 'found' boolean there but I thought someone
will complain about it. Will need to think how this should be done
better. Thanks for the review!
>> + return i;
>> + }
>> + }
>> + }
>> + return -1;
>> +}
>> +
>> void
>> link_assign_uniform_locations(struct gl_shader_program *prog,
>> - unsigned int boolean_true)
>> + unsigned int boolean_true,
>> + unsigned int max_locations)
>> {
>> ralloc_free(prog->UniformStorage);
>> prog->UniformStorage = NULL;
>> @@ -1150,6 +1181,20 @@ link_assign_uniform_locations(struct gl_shader_program *prog,
>>
>> parcel_out_uniform_storage parcel(prog->UniformHash, uniforms, data);
>>
>> + unsigned total_entries = 0;
>> +
>> + /* Calculate amount of 'holes' left after explicit locations were
>> + * reserved from UniformRemapTable.
>> + */
>> + unsigned empty_locs = 0;
>> + for (unsigned i = 0; i < prog->NumUniformRemapTable; i++)
>> + if (prog->UniformRemapTable[i] == NULL)
>> + empty_locs++;
>> +
>> + /* Add all the reserved explicit locations - empty locations in remap table. */
>> + if (prog->NumUniformRemapTable)
>> + total_entries = (prog->NumUniformRemapTable - 1) - empty_locs;
>> +
>> for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>> if (prog->_LinkedShaders[i] == NULL)
>> continue;
>> @@ -1213,21 +1258,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_locations) {
>> + linker_error(prog, "count of uniform locations >= MAX_UNIFORM_LOCATIONS"
>> + "(%u >= %u)", total_entries, max_locations);
>> }
>>
>> /* Reserve all the explicit locations of the active subroutine uniforms. */
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index 6657777..5be8d9f 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -3146,7 +3146,6 @@ check_explicit_uniform_locations(struct gl_context *ctx,
>> return;
>> }
>>
>> - unsigned entries_total = 0;
>> for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
>> struct gl_shader *sh = prog->_LinkedShaders[i];
>>
>> @@ -3158,8 +3157,6 @@ 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;
>> if (var->type->without_array()->is_subroutine())
>> @@ -3173,15 +3170,6 @@ check_explicit_uniform_locations(struct gl_context *ctx,
>> }
>> }
>> }
>> -
>> - /* 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);
>> - }
>> delete uniform_map;
>> }
>>
>> @@ -4556,7 +4544,12 @@ 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,
>> + ctx->Const.MaxUserAssignableUniformLocations);
>> +
>> + if (!prog->LinkStatus)
>> + goto done;
>> +
>> link_assign_atomic_counter_resources(ctx, prog);
>> store_fragdepth_layout(prog);
>>
>> diff --git a/src/glsl/linker.h b/src/glsl/linker.h
>> index c80be1c..76f95c0 100644
>> --- a/src/glsl/linker.h
>> +++ b/src/glsl/linker.h
>> @@ -35,7 +35,8 @@ 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 max_locations);
>>
>> extern void
>> link_set_uniform_initializers(struct gl_shader_program *prog,
>>
>
More information about the mesa-dev
mailing list