[Mesa-dev] [PATCH 04/10] glsl/linker: initialize explicit uniform locations

[Tapani]

On 05/19/2014 07:51 PM, Ian Romanick wrote:
> 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.palli at 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).

OK, will change.

>> +      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)

Yep, makes it more easier to read.

>> +
>> +         /* 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);

ok

>
>> +         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)?

You are right, this needs fixing. I will move initialization of unused 
slots in where allocation is made.
>> +         }
>> +      }
>> +   }
>> +
>> +   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.
>>       */
>>