[Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.

Manolova, Plamena plamena.manolova at intel.com
Thu Feb 11 17:59:16 UTC 2016


Thank you for the review. You're absolutely right, I'll go ahead fix this.

On Thu, Feb 11, 2016 at 6:39 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:

> On Thu, Feb 11, 2016 at 11:31 AM, Plamena Manolova
> <plamena.manolova at 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.palli at intel.com>
> > Signed-off-by: Plamena Manolova <plamena.manolova at intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93696
> > ---
> >  src/compiler/glsl/link_uniforms.cpp | 79
> ++++++++++++++++++++++++++++++++-----
> >  src/compiler/glsl/linker.cpp        | 78
> +++++++++++++++++++++++++-----------
> >  src/compiler/glsl/linker.h          | 17 +++++++-
> >  src/mesa/main/mtypes.h              |  8 ++++
> >  4 files changed, 147 insertions(+), 35 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_uniforms.cpp
> b/src/compiler/glsl/link_uniforms.cpp
> > index 7072c16..8192c21 100644
> > --- a/src/compiler/glsl/link_uniforms.cpp
> > +++ b/src/compiler/glsl/link_uniforms.cpp
> > @@ -1038,9 +1038,36 @@ 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);
> > +
> > +   list_for_each_entry_safe(struct empty_uniform_block, block,
> > +                            &prog->EmptyUniformLocations, link) {
> > +      /* Found a block with enough slots to fit the uniform */
> > +      if (block->slots == entries) {
>
> I haven't fully absorbed this patch, but if you have a bunch of holes
> of 2 slots and then you try to allocate something needing 1 slot, then
> you'll never decide you have a free block right? I think this needs to
> be
>
> block->slots >= entries
>
> and then
>
> if (block->slots > entries) {
>   block->slots -= entries;
>   block->start += entries;
> } else {
>   list_del(&block->link);
>   ralloc_free(block);
> }
>
> Or am I misunderstanding something? This sort of greedy approach
> doesn't guarantee optimal packing, you could do something much fancier
> involving dynamic programming, but that hardly seems necessary.
>
> > +         unsigned start = block->start;
> > +         list_del(&block->link);
> > +         ralloc_free(block);
> > +
> > +         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 +1158,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 +1224,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 +1336,12 @@ link_assign_uniform_locations(struct
> gl_shader_program *prog,
> >
> >     link_set_uniform_initializers(prog, boolean_true);
> >
> > +   list_for_each_entry_safe(struct empty_uniform_block, block,
> > +                            &prog->EmptyUniformLocations, link) {
> > +      ralloc_free(block);
> > +   }
> > +
> > +   list_inithead(&prog->EmptyUniformLocations);
> > +
> >     return;
> >  }
> > diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
> > index bad1c17..cbbb6f1 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,55 @@ 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;
> > +
> > +   list_inithead(&prog->EmptyUniformLocations);
> > +
> > +   struct empty_uniform_block *current_block = NULL;
> > +   unsigned prev_location = -1;
> > +
> > +   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 new continous block of empty
> slots */
> > +         if ((i == 0) || !current_block || (i != prev_location + 1)) {
> > +            current_block = ralloc(NULL, struct empty_uniform_block);
> > +            current_block->start = i;
> > +            current_block->slots = 1;
> > +            list_addtail(&current_block->link,
> &prog->EmptyUniformLocations);
> > +            prev_location = i;
> > +            continue;
> > +         }
> > +
> > +         /* The current block continues, so we simply increment its
> slots */
> > +         current_block->slots++;
> > +         prev_location = i;
> > +      }
> >     }
> > +
> >     delete uniform_map;
> > +   return entries_total;
> >  }
> >
> >  static bool
> > @@ -4129,6 +4156,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
> >
> > @@ -4310,7 +4338,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 +4569,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..9f1fa3e 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 list_head 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..fcc9017 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 "util/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 list_head 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 at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160211/6409abb5/attachment-0001.html>


More information about the mesa-dev mailing list