On Wed, Feb 17, 2016 at 1:22 PM, Manolova, Plamena < plamena.manol...@intel.com> wrote:
> > > 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 - 98303, however if we use total_entries without subtracting 1 we will be comparing to the range 1 - 98304 which will trigger an error. > > > + >> > + 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... >> > Good point I'll fix this so it uses prog as the context. > >> > + 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
