[Mesa-dev] [PATCH 6/7] RFC: nir/xfb_info: arrays of basic types adds just one varying
Alejandro Piñeiro
apinheiro at igalia.com
Fri Nov 9 12:39:15 UTC 2018
On 08/11/18 23:14, Jason Ekstrand wrote:
> On Thu, Nov 8, 2018 at 7:22 AM Alejandro Piñeiro <apinheiro at igalia.com
> <mailto: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];
Yes, GLSL returns one varying per each one (Size 3).
>
> but the following are not
>
> struct S {
> float f;
> vec4 v;
> };
>
> S var[3];
> float var[3][5];
I guess that you are asking for thos two cases because this code is not
handling it properly. You are right. For the array of structs, our code
is crashing. For the array of arrays, it is enumerating four varyings.
One with three GL_FLOAT components, and three with five GL_FLOAT
components, instead of just three varyings with five components. In my
defense, I already mentioned that it was wip code, but preferred to
agree on the way to go before keep working on it.
For the GLSL case, the array of struct returns 6 varyings. And funny
thing, for the array of arrays, GLSL is handling the situation even
worse. It returns the following link error: "Failed to link: error:
Transform feedback varying var[0] undeclared." Just a quick skim on the
spec, I didn't see anything preventing using aoa with transform feedback
varyings, so I guess that this is a bug due all the rules OpenGL has in
relation with variable names.
>
> Is this correct?
Yes. FWIW, I will give you another two examples, from the tests Im using
as reference.
## example 1 ##
struct Array {
float x2_out;
};
struct AoA {
Array x2_Array[2];
};
struct S {
float x1_out;
AoA x2_AoA[2];
float x3_out;
};
layout(xfb_offset = 0) out S s1;
layout(xfb_offset = 0, xfb_buffer = 2) out struct S2 {
float y1_out;
vec4 y2_out;
} s2;
GLSL returns the following varyings (on ARB_gl_spirv we target to get
the same, although without the names)
NumVarying=8, (Offset, Type, BufferIndex, Size, Name)
0:( 0, GL_FLOAT, 0, 1, s1.x1_out)
1:( 4, GL_FLOAT, 0, 1, s1.x2_AoA[0].x2_Array[0].x2_out)
2:( 8, GL_FLOAT, 0, 1, s1.x2_AoA[0].x2_Array[1].x2_out)
3:(12, GL_FLOAT, 0, 1, s1.x2_AoA[1].x2_Array[0].x2_out)
4:(16, GL_FLOAT, 0, 1, s1.x2_AoA[1].x2_Array[1].x2_out)
5:(20, GL_FLOAT, 0, 1, s1.x3_out)
6:( 0, GL_FLOAT, 1, 1, s2.y1_out)
7:( 4, GL_FLOAT_VEC4, 1, 1, s2.y2_out)
## example 2 ##
layout(xfb_offset = 0) out float x1_out;
layout(xfb_offset = 4) out float x2_out[2];
layout(xfb_offset = 12) out vec3 x3_out;
layout(xfb_buffer = 2) out;
layout(xfb_offset = 0, xfb_buffer = 2) out float y1_out;
layout(xfb_offset = 4) out vec4 y2_out
GLSL returns the following varyings (on ARB_gl_spirv we target to get
the same, although without the names)
NumVarying=5, (Offset, Type, BufferIndex, Size, Name)
0:( 0, GL_FLOAT, 0, 1, x1_out)
1:( 4, GL_FLOAT, 0, 2, x2_out)
2:(12, GL_FLOAT_VEC3, 0, 1, x3_out)
3:( 0, GL_FLOAT, 1, 1, y1_out)
4:( 4, GL_FLOAT_VEC4, 1, 1, y2_out)
>
>
> + 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/20181109/37659257/attachment-0001.html>
More information about the mesa-dev
mailing list