Re: [Mesa-dev] [PATCH v2] glsl: move uniform calculation to link_uniforms

[Tapani Pälli]

On 01/20/2016 09:21 PM, Ian Romanick wrote:
So... I'm just going to say that this is commit is the new poster child
for not committing something as soon as you get a positive review.  This
patch went out barely 36 hours ago, and there is already a fix-up patch
for it.  I have also found two other issues in review.

We're not in that much of a hurry.  As one of my engineering professors
used to say, "Fast is slow."

On 01/19/2016 02:17 AM, Tapani Pälli wrote:
Patch moves uniform calculation to happen during link_uniforms, this
is possible with help of UniformRemapTable that has all the reserved
locations.

Location assignment for implicit locations is changed so that we
utilize also the 'holes' that explicit uniform location assignment
might have left in UniformRemapTable, this makes it possible to fit
more uniforms as previously we were lazy here and wasting space.

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

v2: code cleanups (Matt), increment NumUniformRemapTable correctly
     (Timothy), fix find_empty_block to work like intended (sigh) and
     add some more comments.

Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
---
  src/glsl/link_uniforms.cpp | 87 ++++++++++++++++++++++++++++++++++++++++------
  src/glsl/linker.cpp        | 19 ++++------
  src/glsl/linker.h          |  3 +-
  3 files changed, 85 insertions(+), 24 deletions(-)

diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
index 33b2d4c..76ee70d 100644
--- a/src/glsl/link_uniforms.cpp
+++ b/src/glsl/link_uniforms.cpp
@@ -1057,9 +1057,40 @@ assign_hidden_uniform_slot_id(const char *name, unsigned 
hidden_id,
     uniform_size->map->put(hidden_uniform_start + hidden_id, name);
  }

+/**
+ * Search UniformRemapTable for empty block big enough to hold given uniform.
+ * TODO Optimize this algorithm later if it turns out to be a major bottleneck.
+ */
+static int
+find_empty_block(struct gl_shader_program *prog,
+                 struct gl_uniform_storage *uniform)
+{
+   const unsigned entries = MAX2(1, uniform->array_elements);
+   for (unsigned i = 0, j; i < prog->NumUniformRemapTable; i++) {

Please put the declaration of j by itself.  I know it can't go in the
for-loop below, but I think we can spare the extra line of code to make
things more readable.

ok

+      /* We found empty space in UniformRemapTable. */
+      if (prog->UniformRemapTable[i] == NULL) {
+         for (j = i; j < entries && j < prog->NumUniformRemapTable; j++) {
+            if (prog->UniformRemapTable[j] != NULL) {
+               /* Entries do not fit in this space, continue searching
+                * after this location.
+                */
+               i = j + 1;

I'm pretty sure this should be 'i = j'.  Before examining
prog->UniformRemapTable[i] again, i will be incremented.  As a result,
the j+1 element will never be examined.  If j+1 is the start of the only
gap of entries elements, you'll miss it.  Since this is a stand-alone
function, it is a good candidate for some unit tests.

True, this was a last-minute addition and I did not pay enough attention 
to it. We definitely need more tests for this functionality as the CTS 
tests are quite simple since they always leave the 'hole' at the 
beginning of the table.

+               break;
+            }
+         }
+         /* Entries fit, we can return this location. */
+         if (i != j + 1) {

After making the change above, this will also need to change.  It may be
possible to slightly change the inner loop logic.  Maybe:

         /* Not enough space left in the table for the required number
          * of entries.
          */
         if (i + entries > prog->NumUniformRemapTable)
            return -1;

         unsigned gap;
         for (gap = 1; gap < entries; gap++) {
            if (prog->UniformRemapTable[i + gap] != NULL) {
               /* Entries do not fit in this space, continue searching
                * after this location.  The outer loop increment
                * advances past the i+gap element that was already
                * examined.
                */
               gap = 0;
               i += gap;
               break;
            }
         }

         if (gap != 0)
            return i;

Yeah, I originally had extra 'found' boolean there but I thought someone 
will complain about it. Will need to think how this should be done 
better. Thanks for the review!


+            return i;
+         }
+      }
+   }
+   return -1;
+}
+
  void
  link_assign_uniform_locations(struct gl_shader_program *prog,
-                              unsigned int boolean_true)
+                              unsigned int boolean_true,
+                              unsigned int max_locations)
  {
     ralloc_free(prog->UniformStorage);
     prog->UniformStorage = NULL;
@@ -1150,6 +1181,20 @@ link_assign_uniform_locations(struct gl_shader_program 
*prog,

     parcel_out_uniform_storage parcel(prog->UniformHash, uniforms, data);

+   unsigned total_entries = 0;
+
+   /* Calculate amount of 'holes' left after explicit locations were
+    * reserved from UniformRemapTable.
+    */
+   unsigned empty_locs = 0;
+   for (unsigned i = 0; i < prog->NumUniformRemapTable; i++)
+      if (prog->UniformRemapTable[i] == NULL)
+         empty_locs++;
+
+   /* Add all the reserved explicit locations - empty locations in remap 
table. */
+   if (prog->NumUniformRemapTable)
+      total_entries = (prog->NumUniformRemapTable - 1) - empty_locs;
+
     for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
        if (prog->_LinkedShaders[i] == NULL)
         continue;
@@ -1213,21 +1258,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_locations) {
+      linker_error(prog, "count of uniform locations >= MAX_UNIFORM_LOCATIONS"
+                   "(%u >= %u)", total_entries, max_locations);
     }

     /* Reserve all the explicit locations of the active subroutine uniforms. */
diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
index 6657777..5be8d9f 100644
--- a/src/glsl/linker.cpp
+++ b/src/glsl/linker.cpp
@@ -3146,7 +3146,6 @@ check_explicit_uniform_locations(struct gl_context *ctx,
        return;
     }

-   unsigned entries_total = 0;
     for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
        struct gl_shader *sh = prog->_LinkedShaders[i];

@@ -3158,8 +3157,6 @@ 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;
              if (var->type->without_array()->is_subroutine())
@@ -3173,15 +3170,6 @@ check_explicit_uniform_locations(struct gl_context *ctx,
           }
        }
     }
-
-   /* 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);
-   }
     delete uniform_map;
  }

@@ -4556,7 +4544,12 @@ 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,
+                                 ctx->Const.MaxUserAssignableUniformLocations);
+
+   if (!prog->LinkStatus)
+      goto done;
+
     link_assign_atomic_counter_resources(ctx, prog);
     store_fragdepth_layout(prog);

diff --git a/src/glsl/linker.h b/src/glsl/linker.h
index c80be1c..76f95c0 100644
--- a/src/glsl/linker.h
+++ b/src/glsl/linker.h
@@ -35,7 +35,8 @@ 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 max_locations);

  extern void
  link_set_uniform_initializers(struct gl_shader_program *prog,


_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev