[Mesa-dev] [PATCH 6/7] RFC: nir/xfb_info: arrays of basic types adds just one varying
Jason Ekstrand
jason at jlekstrand.net
Fri Nov 9 15:58:48 UTC 2018
On November 9, 2018 06:39:25 Alejandro Piñeiro <apinheiro at igalia.com> wrote:
> 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> 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).
Just to be clear, a matrix it array of matrices is one varying?
>
>
>
>>
>>
>> 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/997f2ec8/attachment-0001.html>
More information about the mesa-dev
mailing list