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

[Ian Romanick]

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

> +      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.
>      */
>