[Mesa-dev] [PATCH 07/12] nir: Add an array splitting pass

[Caio Marcelo de Oliveira Filho]

> > > +static int
> > > +num_arrays_in_type(const struct glsl_type *type)
> > > +{
> > > +   int num_arrays = 0;
> > > +   while (true) {
> > > +      if (glsl_type_is_array(type) || glsl_type_is_matrix(type)) {
> > > +         num_arrays++;
> > > +         type = glsl_get_array_element(type);
> > > +      } else if (glsl_type_is_vector_or_scalar(type)) {
> > > +         return num_arrays;
> > > +      } else {
> > > +         /* Unknown type */
> > > +         return -1;
> >
> > "Unknown" seems misleading, maybe "Unsupported"? The pass uses this to
> > rule out variables that have structs in the hiearchy.
> >
> 
> How about just "Not an array of vectors".  I've also changed the name of
> the helper to "num_arrays_in_array_of_vectors_type"

Sounds good. Thanks.



> > > +struct elem {
> > > +   struct elem *parent;
> > > +
> > > +   const struct glsl_type *type;
> > > +
> > > +   nir_variable *var;
> > > +   struct elem *ind_child;
> > > +   struct elem *children;
> >
> > A small note about what ind_child and children store would be helpful.
> >
> 
> I've completely reworked this data structure in v2.

Ok. I'll check out v2.

(...)


> > > +   bool has_global_splits = false;
> > > +   if (modes & nir_var_global) {
> > > +      has_global_splits = split_var_list_arrays(shader, NULL,
> > > +                                                 &shader->globals,
> > > +                                                 var_indirect_map,
> > > +                                                 var_elem_map, mem_ctx);
> >
> > Optional: remove the extra space in some of the lines above.
> >
> 
> Do you mean the blank lines?  I'm happy to do so, I just don't know what
> way of breaking things up you'd prefer.

The breaking is fine (no strong preferences here).

After the line "has_global_splits ...", there are three lines that
seem to have an extra space. Not really important, is just elsewhere
in the code these are aligned so that those lines would start right
after the '('.

    func(arg1,
          arg2,
          arg3);

vs.

    func(arg1,
         arg2,
         arg3);


Thanks,
Caio