<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Jul 30, 2018 at 12:00 PM Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com">caio.oliveira@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">> > > +static int<br>
> > > +num_arrays_in_type(const struct glsl_type *type)<br>
> > > +{<br>
> > > +   int num_arrays = 0;<br>
> > > +   while (true) {<br>
> > > +      if (glsl_type_is_array(type) || glsl_type_is_matrix(type)) {<br>
> > > +         num_arrays++;<br>
> > > +         type = glsl_get_array_element(type);<br>
> > > +      } else if (glsl_type_is_vector_or_scalar(type)) {<br>
> > > +         return num_arrays;<br>
> > > +      } else {<br>
> > > +         /* Unknown type */<br>
> > > +         return -1;<br>
> ><br>
> > "Unknown" seems misleading, maybe "Unsupported"? The pass uses this to<br>
> > rule out variables that have structs in the hiearchy.<br>
> ><br>
> <br>
> How about just "Not an array of vectors".  I've also changed the name of<br>
> the helper to "num_arrays_in_array_of_vectors_type"<br>
<br>
Sounds good. Thanks.<br>
<br>
<br>
<br>
> > > +struct elem {<br>
> > > +   struct elem *parent;<br>
> > > +<br>
> > > +   const struct glsl_type *type;<br>
> > > +<br>
> > > +   nir_variable *var;<br>
> > > +   struct elem *ind_child;<br>
> > > +   struct elem *children;<br>
> ><br>
> > A small note about what ind_child and children store would be helpful.<br>
> ><br>
> <br>
> I've completely reworked this data structure in v2.<br>
<br>
Ok. I'll check out v2.<br>
<br>
(...)<br>
<br>
<br>
> > > +   bool has_global_splits = false;<br>
> > > +   if (modes & nir_var_global) {<br>
> > > +      has_global_splits = split_var_list_arrays(shader, NULL,<br>
> > > +                                                 &shader->globals,<br>
> > > +                                                 var_indirect_map,<br>
> > > +                                                 var_elem_map, mem_ctx);<br>
> ><br>
> > Optional: remove the extra space in some of the lines above.<br>
> ><br>
> <br>
> Do you mean the blank lines?  I'm happy to do so, I just don't know what<br>
> way of breaking things up you'd prefer.<br>
<br>
The breaking is fine (no strong preferences here).<br>
<br>
After the line "has_global_splits ...", there are three lines that<br>
seem to have an extra space. Not really important, is just elsewhere<br>
in the code these are aligned so that those lines would start right<br>
after the '('.<br>
<br>
    func(arg1,<br>
          arg2,<br>
          arg3);<br>
<br>
vs.<br>
<br>
    func(arg1,<br>
         arg2,<br>
         arg3);<br></blockquote><div><br></div><div>Right.  That appears to be fixed in the latest version.</div><div><br></div><div>--Jason<br></div></div></div>