[Mesa-dev] [PATCH 6/7] RFC: nir/xfb_info: arrays of basic types adds just one varying

[Jason Ekstrand]

On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro <apinheiro at igalia.com>
wrote:

> On OpenGL, a array of a simple type adds just one varying. So
> gl_transform_feedback_varying_info struct defined at mtypes.h includes
> the parameters Type (base_type) and Size (number of elements).
>
> This commit checks this when the recursive add_var_xfb_outputs call
> handles arrays, to ensure that just one is addded.
>
> RFC: Until this point, all changes were reasonable, but this change is
> (imho) ugly. My idea was introducing as less as possible changes on
> the code, specially on its logic/flow. But this commit is almost a
> hack. The ideal solution would be to change the focus of the recursive
> function, focusing on varyings, and at each varying, recursively add
> outputs. But that seems like an overkill for a pass that was
> originally intended for consumers only caring about the outputs. So
> perhaps ARB_gl_spirv should keep their own gathering pass, with
> vayings and outputs, and let this one untouched for those that only
> care on outputs.
> ---
>  src/compiler/nir/nir_gather_xfb_info.c | 52
> ++++++++++++++++++++++++++++------
>  1 file changed, 43 insertions(+), 9 deletions(-)
>
> diff --git a/src/compiler/nir/nir_gather_xfb_info.c
> b/src/compiler/nir/nir_gather_xfb_info.c
> index 948b802a815..cb0e2724cab 100644
> --- a/src/compiler/nir/nir_gather_xfb_info.c
> +++ b/src/compiler/nir/nir_gather_xfb_info.c
> @@ -36,23 +36,59 @@ nir_gather_xfb_info_create(void *mem_ctx, uint16_t
> output_count, uint16_t varyin
>     return xfb;
>  }
>
> +static bool
> +glsl_type_is_leaf(const struct glsl_type *type)
> +{
> +   if (glsl_type_is_struct(type) ||
> +       (glsl_type_is_array(type) &&
> +        (glsl_type_is_array(glsl_get_array_element(type)) ||
> +         glsl_type_is_struct(glsl_get_array_element(type))))) {
>

I'm trying to understand exactly what this means.  From what you wrote here
it looks like the following are all one varying:

float var[3];
vec2 var[3];
mat4 var[3];

but the following are not

struct S {
   float f;
   vec4 v;
};

S var[3];
float var[3][5];

Is this correct?


> +      return false;
> +   } else {
> +      return true;
> +   }
> +}
> +
> +static void
> +add_var_xfb_varying(nir_xfb_info *xfb,
> +                    nir_variable *var,
> +                    unsigned offset,
> +                    const struct glsl_type *type)
> +{
> +   nir_xfb_varying_info *varying = &xfb->varyings[xfb->varying_count++];
> +
> +   varying->type = type;
> +   varying->buffer = var->data.xfb_buffer;
> +   varying->offset = offset;
> +   xfb->buffers[var->data.xfb_buffer].varying_count++;
> +}
> +
>  static void
>  add_var_xfb_outputs(nir_xfb_info *xfb,
>                      nir_variable *var,
>                      unsigned *location,
>                      unsigned *offset,
> -                    const struct glsl_type *type)
> +                    const struct glsl_type *type,
> +                    bool varying_added)
>  {
>     if (glsl_type_is_array(type) || glsl_type_is_matrix(type)) {
>        unsigned length = glsl_get_length(type);
> +      bool local_varying_added = varying_added;
> +
>        const struct glsl_type *child_type = glsl_get_array_element(type);
> +      if (glsl_type_is_leaf(child_type)) {
> +
> +         add_var_xfb_varying(xfb, var, *offset, type);
> +         local_varying_added = true;
> +      }
> +
>        for (unsigned i = 0; i < length; i++)
> -         add_var_xfb_outputs(xfb, var, location, offset, child_type);
> +         add_var_xfb_outputs(xfb, var, location, offset, child_type,
> local_varying_added);
>     } else if (glsl_type_is_struct(type)) {
>        unsigned length = glsl_get_length(type);
>        for (unsigned i = 0; i < length; i++) {
>           const struct glsl_type *child_type = glsl_get_struct_field(type,
> i);
> -         add_var_xfb_outputs(xfb, var, location, offset, child_type);
> +         add_var_xfb_outputs(xfb, var, location, offset, child_type,
> varying_added);
>        }
>     } else {
>        assert(var->data.xfb_buffer < NIR_MAX_XFB_BUFFERS);
> @@ -83,11 +119,9 @@ add_var_xfb_outputs(nir_xfb_info *xfb,
>        uint8_t comp_mask = ((1 << comp_slots) - 1) <<
> var->data.location_frac;
>        unsigned location_frac = var->data.location_frac;
>
> -      nir_xfb_varying_info *varying =
> &xfb->varyings[xfb->varying_count++];
> -      varying->type = type;
> -      varying->buffer = var->data.xfb_buffer;
> -      varying->offset = *offset;
> -      xfb->buffers[var->data.xfb_buffer].varying_count++;
> +      if (!varying_added) {
> +         add_var_xfb_varying(xfb, var, *offset, type);
> +      }
>
>        assert(attrib_slots <= 2);
>        for (unsigned s = 0; s < attrib_slots; s++) {
> @@ -171,7 +205,7 @@ nir_gather_xfb_info(const nir_shader *shader, void
> *mem_ctx)
>
>           unsigned location = var->data.location;
>           unsigned offset = var->data.offset;
> -         add_var_xfb_outputs(xfb, var, &location, &offset, var->type);
> +         add_var_xfb_outputs(xfb, var, &location, &offset, var->type,
> false);
>        }
>     }
>     assert(xfb->output_count == num_outputs);
> --
> 2.14.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181108/f684d73d/attachment.html>