[Mesa-dev] [PATCH] glsl: add varying resources for arrays of complex types

[Ilia Mirkin]

On Fri, Oct 27, 2017 at 11:07 AM, Juan A. Suarez Romero
<jasuarez at igalia.com> wrote:
> On Fri, 2017-10-27 at 10:27 -0400, Ilia Mirkin wrote:
>> On Fri, Oct 27, 2017 at 10:03 AM, Juan A. Suarez Romero
>> <jasuarez at igalia.com> wrote:
>> > This patch is mostly a patch done by Ilia Mirkin.
>> >
>> > It fixes KHR-GL45.enhanced_layouts.varying_structure_locations.
>> >
>> > CC: Ilia Mirkin <imirkin at alum.mit.edu>
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103098
>> > Signed-off-by: Juan A. Suarez Romero <jasuarez at igalia.com>
>> > ---
>> >  src/compiler/glsl/linker.cpp | 36 ++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 36 insertions(+)
>> >
>> > diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> > index f827b68555..7305b7740b 100644
>> > --- a/src/compiler/glsl/linker.cpp
>> > +++ b/src/compiler/glsl/linker.cpp
>> > @@ -3882,6 +3882,42 @@ add_shader_variable(const struct gl_context *ctx,
>> >        return true;
>> >     }
>> >
>> > +   case GLSL_TYPE_ARRAY: {
>> > +      /* The ARB_program_interface_query spec says:
>> > +       *
>> > +       *     "For an active variable declared as an array of basic types, a
>> > +       *      single entry will be generated, with its name string formed by
>> > +       *      concatenating the name of the array and the string "[0]"."
>> > +       *
>> > +       *     "For an active variable declared as an array of an aggregate data
>> > +       *      type (structures or arrays), a separate entry will be generated
>> > +       *      for each active array element, unless noted immediately below.
>> > +       *      The name of each entry is formed by concatenating the name of
>> > +       *      the array, the "[" character, an integer identifying the element
>> > +       *      number, and the "]" character.  These enumeration rules are
>> > +       *      applied recursively, treating each enumerated array element as a
>> > +       *      separate active variable."
>> > +       */
>>
>> So if you have a TCS or TES or GS with
>>
>> in struct { vec4 foo; } bar[4];
>>
>> Wouldn't this add bar[0].foo, bar[1].foo, bar[2].foo, and bar[3].foo?
>
> According to the spec, yes. Unless I'm missing something...
>
>
> Looking for other examples, in
>
> https://www.khronos.org/opengl/wiki/Program_Introspection#Arrays
>
> it shows a similar example, and also do as you say.
>
>> With the latter ones getting bogus locations? What is it supposed to
>> do in this case?
>
> Why it would get bogus locations?

Because it'll do elem_location += stride every time, but they each
should get the same location.

>
>
>>
>> > +      const struct glsl_type *array_type = type->fields.array;
>> > +      if (array_type->base_type == GLSL_TYPE_STRUCT ||
>> > +          array_type->base_type == GLSL_TYPE_ARRAY) {
>> > +         unsigned elem_location = location;
>> > +         unsigned stride = array_type->count_attribute_slots(false);
>> > +         for (unsigned i = 0; i < type->length; i++) {
>> > +            char *elem = ralloc_asprintf(shProg, "%s[%d]", name, i);
>> > +            if (!add_shader_variable(ctx, shProg, resource_set,
>> > +                                     stage_mask, programInterface,
>> > +                                     var, elem, array_type,
>> > +                                     use_implicit_location, elem_location,
>> > +                                     outermost_struct_type))
>> > +               return false;
>> > +            elem_location += stride;
>> > +         }
>> > +         return true;
>> > +      }
>> > +      /* fallthrough */
>> > +   }
>> > +
>> >     default: {
>> >        /* The ARB_program_interface_query spec says:
>> >         *
>> > --
>> > 2.13.6
>> >
>>
>>