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

[Jason Ekstrand]

On Mon, Jul 30, 2018 at 12:00 PM Caio Marcelo de Oliveira Filho <
caio.oliveira at intel.com> wrote:

> > > > +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);
>

Right.  That appears to be fixed in the latest version.

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180731/7573785c/attachment-0001.html>