[Mesa-dev] [PATCH 07/20] glsl: clean-up link uniform code

[Timothy Arceri]

On Wed, 2015-07-29 at 10:14 -0400, Ilia Mirkin wrote:
> On Wed, Jul 29, 2015 at 9:56 AM, Timothy Arceri <t_arceri at yahoo.com.au> 
> wrote:
> > These changes are also needed to allow linking of
> > struct and interface arrays of arrays.
> > ---
> >  src/glsl/link_uniforms.cpp | 16 ++++++----------
> >  1 file changed, 6 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/glsl/link_uniforms.cpp b/src/glsl/link_uniforms.cpp
> > index 254086d..42f75e9 100644
> > --- a/src/glsl/link_uniforms.cpp
> > +++ b/src/glsl/link_uniforms.cpp
> > @@ -72,6 +72,7 @@ void
> >  program_resource_visitor::process(ir_variable *var)
> >  {
> >     const glsl_type *t = var->type;
> > +   const glsl_type *t_without_array = var->type->without_array();
> >     const bool row_major =
> >        var->data.matrix_layout == GLSL_MATRIX_LAYOUT_ROW_MAJOR;
> > 
> > @@ -141,12 +142,8 @@ program_resource_visitor::process(ir_variable *var)
> >        char *name = ralloc_strdup(NULL, var->name);
> >        recursion(var->type, &name, strlen(name), row_major, NULL, false);
> >        ralloc_free(name);
> > -   } else if (t->is_interface()) {
> > -      char *name = ralloc_strdup(NULL, var->type->name);
> > -      recursion(var->type, &name, strlen(name), row_major, NULL, false);
> > -      ralloc_free(name);
> > -   } else if (t->is_array() && t->fields.array->is_interface()) {
> > -      char *name = ralloc_strdup(NULL, var->type->fields.array->name);
> > +   } else if (t_without_array->is_interface()) {
> > +      char *name = ralloc_strdup(NULL, t_without_array->name);
> >        recursion(var->type, &name, strlen(name), row_major, NULL, false);
> >        ralloc_free(name);
> >     } else {
> > @@ -217,8 +214,8 @@ program_resource_visitor::recursion(const glsl_type 
> > *t, char **name,
> >           (*name)[name_length] = '\0';
> >           this->leave_record(t, *name, row_major);
> >        }
> > -   } else if (t->is_array() && (t->fields.array->is_record()
> > -                                || t->fields.array->is_interface())) {
> > +   } else if (t->without_array()->is_record()
> > +              || t->without_array()->is_interface()) {
> >        if (record_type == NULL && t->fields.array->is_record())
> >           record_type = t->fields.array;
> 
> Presumably here you want
> 
> if (record_type == NULL)
>   record_type = t->without_array();

Nope it doesn't matter it only needs to be set for the inner most array, if we
leave it as NULL it will get set for the innermost array by the recurrsive
calls.

> 
> Also in e.g. parcel_out_uniform_storage::visit_field, there's code like
> 
>       if (type->is_array()) {
>          this->uniforms[id].array_elements = type->length;
>          base_type = type->fields.array;
> 
> and base_type is then used to determine things like if it's a
> sampler/etc. Not sure if you fix that up elsewhere.

The code is intended to be left like this, see the commit message in patch 8
for more details. 

Originally (v1 changed in v2) I had the code working the way your thinking it
should be as the spec doesn't really give any details on how things should be
implemented.

However after seeing more details in the issues section of the program
interface query spec it became more clear how its intended to be, basically
all but the innermost array is treated as a separate uniform, eventually it
would make sense to have a pass the splits the arrays and does some dead code
elimination, the Nvidia driver is pretty aggressive with AoA elimination
(sometimes too aggressive from my piglit tests). 

> 
> I highly recommend taking Ian's random_ubo.sh script
> (http://cgit.freedesktop.org/~idr/piglit/log/?h=ubo-lolz), and fixing
> it up for arrays-of-arrays. It helped me find a bunch of issues in the
> fp64 stuff. [Note that the script itself has some fp64-related
> issues... I should probably push my fixes somewhere.]

Thanks for pointing out the exact script you mentioned it once before but I
forgot to look for it :)

> 
> > 
> > @@ -810,8 +807,7 @@ link_update_uniform_buffer_variables(struct gl_shader 
> > *shader)
> > 
> >        if (var->type->is_record()) {
> >           sentinel = '.';
> > -      } else if (var->type->is_array()
> > -                 && var->type->fields.array->is_record()) {
> > +      } else if (var->type->without_array()->is_record()) {
> >           sentinel = '[';
> >        }
> > 
> > --
> > 2.4.3
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev