[Mesa-dev] [PATCH v2 13/15] glsl linker: support arrays of interface block instances

[Pohjolainen, Topi]

On Sat, Mar 23, 2013 at 01:48:44PM -0700, Kenneth Graunke wrote:
> On 03/19/2013 03:57 AM, Pohjolainen, Topi wrote:
> >On Mon, Mar 18, 2013 at 04:35:10PM -0700, Jordan Justen wrote:
> >>With this change we now support interface block arrays.
> >>For example, cases like this:
> >>
> >>out block_name {
> >>     float f;
> >>} block_instance[2];
> >>
> >>This allows Mesa to pass the piglit glsl-1.50 test:
> >>* execution/interface-blocks-complex-vs-fs.shader_test
> >>
> >>Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> >>---
> >>  src/glsl/lower_named_interface_blocks.cpp |   53 ++++++++++++++++++++++++-----
> >>  1 file changed, 44 insertions(+), 9 deletions(-)
> >>
> >>diff --git a/src/glsl/lower_named_interface_blocks.cpp b/src/glsl/lower_named_interface_blocks.cpp
> >>index 2e0c322..405e7a9 100644
> >>--- a/src/glsl/lower_named_interface_blocks.cpp
> >>+++ b/src/glsl/lower_named_interface_blocks.cpp
> >>@@ -107,22 +107,47 @@ flatten_named_interface_blocks_declarations::run(exec_list *instructions)
> >>           if (var->mode == ir_var_uniform)
> >>              continue;
> >>
> >>-         const glsl_type *const t = var->type;
> >>+         const glsl_type * iface_t = var->type;
> >>+         const glsl_type * array_t = NULL;
> >>           exec_node *insert_pos = var;
> >>-         char *iface_field_name;
> >>-         for (unsigned i = 0; i < t->length; i++) {
> >>-            iface_field_name = ralloc_asprintf(mem_ctx, "%s.%s", t->name,
> >>-                                               t->fields.structure[i].name);
> >>+
> >>+         if (iface_t->is_array()) {
> >>+            array_t = iface_t;
> >>+            iface_t = array_t->fields.array;
> >>+         }
> >>+
> >>+         assert (iface_t->is_interface());
> >>+
> >>+         for (unsigned i = 0; i < iface_t->length; i++) {
> >>+            const char * field_name = iface_t->fields.structure[i].name;
> >>+            char *iface_field_name =
> >>+               ralloc_asprintf(mem_ctx, "%s.%s",
> >>+                               iface_t->name, field_name);
> >>
> >>              ir_variable *found_var =
> >>                 (ir_variable *) hash_table_find(interface_namespace,
> >>                                                 iface_field_name);
> >>              if (!found_var) {
> >>                 ir_variable *new_var =
> >>-                  new(mem_ctx) ir_variable(t->fields.structure[i].type,
> >>-                                           ralloc_strdup(mem_ctx, t->fields.structure[i].name),
> >>+                  new(mem_ctx) ir_variable(iface_t->fields.structure[i].type,
> >>+                                           ralloc_strdup(mem_ctx, iface_t->fields.structure[i].name),
> >>                                             (ir_variable_mode) var->mode);
> >>-               new_var->interface_type = t;
> >>+               if (array_t != NULL) {
> >>+                  const glsl_type *new_array_type =
> >>+                     glsl_type::get_array_instance(
> >>+                        iface_t->fields.structure[i].type,
> >>+                        array_t->length);
> >>+                  char *array_var_name =
> >>+                     ralloc_asprintf(mem_ctx, "%s[%d]",
> >>+                                     new_var->name, array_t->length);
> >>+                  ir_variable *new_array_var =
> >>+                     new(mem_ctx) ir_variable(new_array_type,
> >>+                                              array_var_name,
> >>+                                              (ir_variable_mode) var->mode);
> >>+                  new_var = new_array_var;
> >
> >Don't you leak the previously allocated instance of 'ir_variable' (assigned to
> >'new_var')? Or is it just left until 'mem_ctx' gets released?
> >I'm not that familiar with the glsl core that I may well be missing something.
> 
> This is actually fairly common in the GLSL code - we routinely just
> drop allocated memory on the floor.  But, it all works out thanks to
> the magic of talloc/ralloc(*).
> 
> With talloc, you allocate memory out af a 'context'.  Freeing a
> context frees any memory associated with it, which means you can
> free a whole bunch of things without tracking them all down and
> calling free() on each individual pointer.  It also means you don't
> need to explicitly keep pointers to each object, as the memory
> context does that for you.
> 
> Any talloced piece of memory is also a new context, which allows you
> to create a tree-like structure (the 't' in talloc is for 'tree',
> and the 'r' in ralloc is for 'recursive').
> 
> In the GLSL compiler, we allocate IR out of either the parser state
> or the gl_shader object (I forget which).  During optimization,
> linking, and so on, we create new IR, and remove other IR, all
> without worrying about memory.  Then, when we're done
> compiling/linking, we walk through the remaining IR tree, calling
> talloc_steal() on each remaining IR node to reparent the memory to a
> second memory context.  Then we free the original memory context,
> which now contains only the junk we don't need anymore.
> 
> (*) ralloc is my poor man's reimplementation of talloc, licensed
> under the MIT license rather than LGPLv3.  It also has a slightly
> different API and performance characteristics.

Yes, I took a look at the implemention just after asking about it, and
understood that it is indeed the 'mem_ctx' bookkeeping that this "dropping on
the floor" is relying on as you explained. The implentation does, however, 
allow one to use it without a context (mem_ctx == 0) and in that case there
would be nothing keeping track and hence the things "on the floor" getting
leaked. But I assume there is nothing using this logic without a proper memory
context.