On Mon, Jul 30, 2018 at 12:00 PM Caio Marcelo de Oliveira Filho <
caio.olive...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
