On Mon, Feb 15, 2016 at 10:50 PM, Ilia Mirkin <imir...@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). > Thanks for spotting this, it must've slipped by me. > > On Fri, Feb 12, 2016 at 7:38 AM, Plamena Manolova > <plamena.manol...@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.pa...@intel.com> > > Signed-off-by: Plamena Manolova <plamena.manol...@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? > According to the spec: https://www.opengl.org/registry/specs/ARB/explicit_uniform_location.txt: "The explicitly defined locations and the generated locations must be in the range of 0 to MAX_UNIFORM_LOCATIONS minus one.", which means that the uniforms must fit between array slots 0 - 98304, however if we use > + > > + 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... > > > + current_block->start = i; > > + exec_list_push_tail(&prog->EmptyUniformLocations, > > + ¤t_block->link); > > + } > > + > > + /* The current block continues, so we simply increment its > slots */ > > + current_block->slots++; > > + } > > } > > + > > delete uniform_map; > > + return entries_total; > > } > > > > static bool > > @@ -4129,6 +4151,7 @@ link_shaders(struct gl_context *ctx, struct > gl_shader_program *prog) > > > > tfeedback_decl *tfeedback_decls = NULL; > > unsigned num_tfeedback_decls = prog->TransformFeedback.NumVarying; > > + unsigned int num_explicit_uniform_locs = 0; > > > > void *mem_ctx = ralloc_context(NULL); // temporary linker context > > This is the ralloc context you should use... and then you don't have > to worry about freeing stuff "by hand". Just drop it and it'll get > magically cleaned up at the end. > > > > > @@ -4310,7 +4333,7 @@ link_shaders(struct gl_context *ctx, struct > gl_shader_program *prog) > > last = i; > > } > > > > - check_explicit_uniform_locations(ctx, prog); > > + num_explicit_uniform_locs = check_explicit_uniform_locations(ctx, > prog); > > link_assign_subroutine_types(prog); > > > > if (!prog->LinkStatus) > > @@ -4541,7 +4564,9 @@ 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, > > + num_explicit_uniform_locs, > > + > ctx->Const.MaxUserAssignableUniformLocations); > > link_assign_atomic_counter_resources(ctx, prog); > > store_fragdepth_layout(prog); > > > > diff --git a/src/compiler/glsl/linker.h b/src/compiler/glsl/linker.h > > index c80be1c..a60bb6e 100644 > > --- a/src/compiler/glsl/linker.h > > +++ b/src/compiler/glsl/linker.h > > @@ -35,7 +35,9 @@ 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 num_explicit_uniform_locs, > > + unsigned int max_uniform_locs); > > > > extern void > > link_set_uniform_initializers(struct gl_shader_program *prog, > > @@ -202,4 +204,17 @@ linker_error(gl_shader_program *prog, const char > *fmt, ...); > > void > > linker_warning(gl_shader_program *prog, const char *fmt, ...); > > > > +/** > > + * Sometimes there are empty slots left over in UniformRemapTable after > we > > + * allocate slots to explicit locations. This struct represents a single > > + * continouous block of empty slots in UniformRemapTable. > > + */ > > +struct empty_uniform_block { > > + struct exec_node link; > > + /* The start location of the block */ > > + unsigned start; > > + /* The number of slots in the block */ > > + unsigned slots; > > +}; > > + > > #endif /* GLSL_LINKER_H */ > > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h > > index e987177..aeb8043 100644 > > --- a/src/mesa/main/mtypes.h > > +++ b/src/mesa/main/mtypes.h > > @@ -44,6 +44,7 @@ > > #include "math/m_matrix.h" /* GLmatrix */ > > #include "compiler/shader_enums.h" > > #include "main/formats.h" /* MESA_FORMAT_COUNT */ > > +#include "compiler/glsl/list.h" > > > > > > #ifdef __cplusplus > > @@ -2764,6 +2765,13 @@ struct gl_shader_program > > struct gl_uniform_storage **UniformRemapTable; > > > > /** > > + * Sometimes there are empty slots left over in UniformRemapTable > after we > > + * allocate slots to explicit locations. This list stores the blocks > of > > + * continuous empty slots inside UniformRemapTable. > > + */ > > + struct exec_list EmptyUniformLocations; > > + > > + /** > > * Size of the gl_ClipDistance array that is output from the last > pipeline > > * stage before the fragment shader. > > */ > > -- > > 2.4.3 > > > > --------------------------------------------------------------------- > > Intel Corporation (UK) Limited > > Registered No. 1134945 (England) > > Registered Office: Pipers Way, Swindon SN3 1RJ > > VAT No: 860 2173 47 > > > > This e-mail and any attachments may contain confidential material for > > the sole use of the intended recipient(s). Any review or distribution > > by others is strictly prohibited. If you are not the intended > > recipient, please contact the sender and delete all copies. > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > --------------------------------------------------------------------- > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
