[Mesa-dev] [PATCH 03/25] glsl: clean-up link uniform code

[Timothy Arceri]

On Thu, 2015-08-20 at 10:39 -0700, Ian Romanick wrote:
> On 08/20/2015 10:31 AM, Ian Romanick wrote:
> > On 08/19/2015 09:37 PM, Timothy Arceri 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;
> > 
> > Is the part after the change still correct?  If t is an array of array
> > of struct, record_type will still point at NULL.  I guess this might be
> > addressed in a later patch...
> 
> It looks like it's somewhat addressed in the next patch.  It's not clear
> why this is better than just adding a "|| t->fields.array->is_array()"
> clause to the right side of the &&.  Is that just the way it turned out
> (which is fine), or am I missing something subtle (which should probably
> be documented)?

Please correct me if I'm missing something but I don't think the next patch
changes anything in regards to setting record_type. The change in the
following patch is about treating all but the innermost array for AoA like we
currently would treat nested struct arrays.

Ilia also questioned this code in an earlier series so maybe I should add a
comment. Having record_type point at NULL shouldn't matter for AoA of structs
because we should just end up back in this same piece of code i.e it will
eventually get set for the innermost array by the recurrsive calls. 

Adding || t->fields.array->is_array() would set record_type to the outermost
arrays element type which I don't think is what we want right? The outer
arrays are meant to be treated as separate uniforms, we only care about the
innermost array.

> 
> > > @@ -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 = '[';
> > >        }
> > >  
> > > 
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > 
>