[Mesa-dev] [PATCH] glsl: move uniform calculation to link_uniforms
Timothy Arceri
t_arceri at yahoo.com.au
Sat Jan 16 21:36:56 PST 2016
On Sun, 2016-01-17 at 07:09 +0200, Tapani Pälli wrote:
> On 01/17/2016 04:56 AM, Timothy Arceri wrote:
> > On Sat, 2016-01-16 at 07:40 +0200, Tapani Pälli wrote:
> > > Patch moves uniform calculation to happen during link_uniforms,
> > Hi Tapani,
> >
> > I like that you are moving the check here as it allows arrays
> > without
> > an explicit location to be resized before the max check is done.
> > However I'm not too excited about looping over the remap table once
> > again to calculate counts, how about doing something like this.
> >
> > Change both:
> >
> > reserve_explicit_locations() and maybe
> > reserve_subroutine_explicit_locations() (see below)
> >
> > to return -1 on failure and the value of slots on pass, use this
> > rather
> > than the existing:
> >
> > entries_total += var->type->uniform_locations();
> >
> > to build the count in check_explicit_uniform_locations(), this is
> > important because subroutines and regular uniforms have a separate
> > count/limit.
> >
> > Then edit check_explicit_uniform_locations() to return the explicit
> > locations count then pass this to link_assign_uniform_locations()
>
> IMO it is cleaner to have it in one place, much easier to handle
> issues
> when the whole calculation is done here.
I disagree so one last attempt to convert you :P. The whole calculation
is not really done here, your creating the explicit remap table
elsewhere and just counting it here. IMO its cleaner to count the
explicit locations where you handle them then pass the count here and
add the implicit locations to it.
This way you are keeping the counting of the explicit/implicit location
in the same place you are handling them.
> > I wonder if the max subroutine check check_subroutine_resources()
> > has
> > the same problem with counting explicit locations? Maybe we need to
> > use
> > a new value with that check too? Guess we need a test.
>
> This is quite likely, there is bug 93731 that hints there is
> something
> wrong with them. I'd like to handle that as separate issue.
>
> > unsigned explicit_subroutine_count = 0;
> > unsigned explicit_uniform_count = 0;
> > check_explicit_uniform_locations(prog, &explicit_uniform_count,
> > &explicit_subroutine_count);
> >
> >
> >
> >
> > > 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
> > >
> > > Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> > > ---
> > > src/glsl/link_uniforms.cpp | 78
> > > ++++++++++++++++++++++++++++++++++++++++------
> > > src/glsl/linker.cpp | 19 ++++-------
> > > src/glsl/linker.h | 3 +-
> > > 3 files changed, 77 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/src/glsl/link_uniforms.cpp
> > > b/src/glsl/link_uniforms.cpp
> > > index 33b2d4c..91e973a 100644
> > > --- a/src/glsl/link_uniforms.cpp
> > > +++ b/src/glsl/link_uniforms.cpp
> > > @@ -1057,9 +1057,31 @@ 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
> > > +seek_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++)
> > > + if (prog->UniformRemapTable[i] == NULL) {
> > > + for (j = i; j < entries && j < prog
> > > ->NumUniformRemapTable;
> > > j++)
> > > + if (prog->UniformRemapTable[j] != NULL) {
> > > + i = j;
> > > + break;
> > > + }
> > > + 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 +1172,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;
> > No need for the += just =
> >
> > Although if you follow my above suggestion you shouldn't even need
> > most
> > of the above code it would just be:
> >
> > unsigned total_entries = explicit_uniform_count;
> >
> > Or just use the variable directly.
>
> My vote is to keep all calculation here. Most of the time we are
> dealing
> with 'tens' of uniforms, not thousands like crazy conformance tests
> do,
> this is fast loop.
>
> > > +
> > > for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> > > if (prog->_LinkedShaders[i] == NULL)
> > > continue;
> > > @@ -1213,23 +1249,47 @@ 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 = seek_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);
> > > + }
> > >
> > > /* 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;
> > Now that you are doing a seek you need to move the above line into
> > the
> > else block along with the resize of the prog->UniformRemapTable
> > right?
>
> Good catch, thanks! Will fix.
>
> > > }
> > >
> > > + /* 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. */
> > > for (unsigned i = 0; i < num_uniforms; i++) {
> > > if (!uniforms[i].type->is_subroutine())
> > > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
> > > index 3f40c0b..ff33fa1 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->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