<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Aug 20, 2018 at 6:41 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">On Sat, Jul 28, 2018 at 10:44:36PM -0700, Jason Ekstrand wrote:<br>
> This pass doesn't really do much now because nir_lower_vars_to_ssa can<br>
> already see through structures and considers them to be "split".  This<br>
> pass exists to help other passes more easily see through structure<br>
> variables.  If a back-end does implement arrays using scratch or<br>
> indirects on registers, having more smaller arrays is likely to have<br>
> better memory efficiency.<br>
> ---<br>
>  src/compiler/Makefile.sources     |   1 +<br>
>  src/compiler/nir/meson.build      |   1 +<br>
>  src/compiler/nir/nir.h            |   1 +<br>
>  src/compiler/nir/nir_split_vars.c | 271 ++++++++++++++++++++++++++++++<br>
>  4 files changed, 274 insertions(+)<br>
>  create mode 100644 src/compiler/nir/nir_split_vars.c<br>
<br>
With the fix below, this patch is<br>
<br>
Reviewed-by: Caio Marcelo de Oliveira Filho <<a href="mailto:caio.oliveira@intel.com" target="_blank">caio.oliveira@intel.com</a>><br>
<br>
<br>
> +static void<br>
> +init_field_for_type(struct field *field, struct field *parent,<br>
> +                    const struct glsl_type *type,<br>
> +                    const char *name,<br>
> +                    struct split_var_state *state)<br>
> +{<br>
> +   *field = (struct field) {<br>
> +      .parent = parent,<br>
> +      .type = type,<br>
> +   };<br>
> +<br>
> +   const struct glsl_type *struct_type = glsl_without_array(type);<br>
> +   if (glsl_type_is_struct(struct_type)) {<br>
> +      field->num_fields = glsl_get_length(struct_type),<br>
> +      field->fields = ralloc_array(state->mem_ctx, struct field,<br>
> +                                   field->num_fields);<br>
> +      for (unsigned i = 0; i < field->num_fields; i++) {<br>
> +         char *field_name = NULL;<br>
> +         if (name) {<br>
> +            ralloc_asprintf(state->mem_ctx, "%s_%s", name,<br>
> +                            glsl_get_struct_elem_name(struct_type, i));<br>
<br>
Store the result of asprintf:<br>
<br>
    field_name = ralloc_asprintf(...);<br></blockquote><div><br></div><div>Fixed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
And from previous review:<br>
<br>
    Maybe if no name for the parent is available, use something in<br>
    this place ("unnamed", or whatever).  That way the rest of the<br>
    hierarchy doesn't lose the meaning completely.<br></blockquote><div><br></div><div>I've made it name the variable "{unnamed $type}" where $type is the name of the structure type.</div><div><br></div><div>--Jason<br></div></div></div>