On 04/09/2014 02:56 AM, Tapani Pälli wrote: > Patch initializes the UniformRemapTable for explicit locations. This > needs to happen before optimizations to make sure all inactive uniforms > get their explicit locations correctly. > > Signed-off-by: Tapani Pälli <tapani.pa...@intel.com> > --- > src/glsl/linker.cpp | 99 > +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 99 insertions(+) > > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp > index 7c194a2..1b4cb63 100644 > --- a/src/glsl/linker.cpp > +++ b/src/glsl/linker.cpp > @@ -74,6 +74,7 @@ > #include "link_varyings.h" > #include "ir_optimization.h" > #include "ir_rvalue_visitor.h" > +#include "ir_uniform.h" > > extern "C" { > #include "main/shaderobj.h" > @@ -2089,6 +2090,100 @@ check_image_resources(struct gl_context *ctx, struct > gl_shader_program *prog) > linker_error(prog, "Too many combined image uniforms and fragment > outputs"); > } > > + > +/** > + * Initializes explicit location slots point to -1 for a variable, > + * checks for overlaps between other uniforms using explicit locations. > + */ > +static bool > +reserve_explicit_locations(struct gl_shader_program *prog, > + string_to_uint_map *map, ir_variable *var) > +{ > + unsigned max_loc = var->data.location + var->type->component_slots() - 1; > + > + /* Resize remap table if locations do not fit in the current one. */ > + if (max_loc + 1 > prog->NumUniformRemapTable) { > + prog->UniformRemapTable = > + reralloc(prog, prog->UniformRemapTable, > + gl_uniform_storage *, > + max_loc + 1); > + prog->NumUniformRemapTable = max_loc + 1; > + } > + > + for (unsigned i = 0; i < var->type->component_slots(); i++) {
You should check the code that gets generated for this. I suspect this will translate to a call to component_slots per iteration of the loop. Maybe just call it once above (since it is also used to calculate max_loc). > + unsigned loc = var->data.location + i; > + > + /* Check if location is already used. */ > + if (prog->UniformRemapTable[loc] == (gl_uniform_storage *) -1) { So... -1 means that an inactive uniform has that location explicitly assigned? I'm inferring that from comments in the next patch. Maybe we should have a descriptive #define #define INACTIVE_UNIFORM_EXPLICIT_LOCATION ((gl_uniform_storage *) -1) > + > + /* 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; > + > + /* ARB_explicit_uniform_location specification states: > + * > + * "No two default-block uniform variables in the program can > have > + * the same location, even if they are unused, otherwise a > compiler > + * or linker error will be generated." > + */ > + linker_error(prog, "location qualifier " > + "for uniform %s " > + "overlaps previously used location", > + var->name); Minor nit (which you can take or leave). I usually like to have fewer breaks in strings. I would have split this up as: linker_error(prog, "location qualifier for uniform %s overlaps " "previously used location", var->name); > + return false; > + } > + > + prog->UniformRemapTable[loc] = (gl_uniform_storage *) -1; > + } > + > + /* Note, base location used for arrays. */ > + map->put(var->data.location, var->name); > + > + return true; > +} > + > +/** > + * Check and reserve all explicit uniform locations, called before > + * any optimizations happen to handle also inactive uniforms and > + * inactive array elements that may get trimmed away. > + */ > +static void > +check_explicit_uniform_locations(struct gl_context *ctx, > + struct gl_shader_program *prog) > +{ > + if (!ctx->Extensions.ARB_explicit_uniform_location) > + return; > + > + /* This map is used to detect if overlapping explicit locations > + * occur with the same uniform (from different stage) or a different one. > + */ > + string_to_uint_map *uniform_map = new string_to_uint_map; > + > + for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) { > + struct gl_shader *sh = prog->_LinkedShaders[i]; > + > + if (!sh) > + continue; > + > + foreach_list(node, sh->ir) { > + ir_variable *var = ((ir_instruction *)node)->as_variable(); > + if ((var && var->data.mode == ir_var_uniform) && > + var->data.explicit_location) { > + if (!reserve_explicit_locations(prog, uniform_map, var)) > + return; > + > + /* Initialize locations that were allocated but left unused. */ > + for (unsigned i = 0; i < prog->NumUniformRemapTable; i++) > + if (prog->UniformRemapTable[i] != (gl_uniform_storage *) -1) > + prog->UniformRemapTable[i] = NULL; I'm confused by this... what's in UniformRemapTable on entry to this function? Random garbage (this might happen to be ~0)? > + } > + } > + } > + > + delete uniform_map; > +} > + > void > link_shaders(struct gl_context *ctx, struct gl_shader_program *prog) > { > @@ -2232,6 +2327,10 @@ link_shaders(struct gl_context *ctx, struct > gl_shader_program *prog) > break; > } > > + check_explicit_uniform_locations(ctx, prog); > + if (!prog->LinkStatus) > + goto done; > + > /* Validate the inputs of each stage with the output of the preceding > * stage. > */ > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev