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.pa...@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.
> + /* 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.
> + 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;
> + 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,
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
