[Mesa-dev] [PATCH] glsl: look for frag data bindings with [0] tacked onto the end for arrays

[Ilia Mirkin]

Thanks for confirming, Corentin.

Ian, do you have any opinions on this? Seems like a fairly innocuous
thing to be doing...

On Fri, Jul 8, 2016 at 3:39 PM, Corentin Wallez <corentin at wallez.net> wrote:
> Not sure how reviews work in Mesa, but this patch LGTM. I also tested that
> it fixes the relevant tests failures it is supposed to address.
>
> On Wed, Jul 6, 2016 at 7:40 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>
>> The GL spec is very unclear on this point. Apparently this is discussed
>> without resolution in the closed Khronos bugtracker at
>> https://cvs.khronos.org/bugzilla/show_bug.cgi?id=7829 . The
>> recommendation is to allow dropping the [0] for looking up the bindings.
>>
>> The approach taken in this patch is to instead tack on [0]'s for each
>> arrayness level of the output's type, and doing the lookup again. That
>> way, for
>>
>> out vec4 foo[2][2][2]
>>
>> we will end up looking for bindings for foo, foo[0], foo[0][0], and
>> foo[0][0][0], in that order of preference.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96765
>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> ---
>>  src/compiler/glsl/linker.cpp | 39 ++++++++++++++++++++++++++++-----------
>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
>> index d963f54..9d54c2f 100644
>> --- a/src/compiler/glsl/linker.cpp
>> +++ b/src/compiler/glsl/linker.cpp
>> @@ -2566,6 +2566,7 @@ find_available_slots(unsigned used_mask, unsigned
>> needed_count)
>>  /**
>>   * Assign locations for either VS inputs or FS outputs
>>   *
>> + * \param mem_ctx       Temporary ralloc context used for linking
>>   * \param prog          Shader program whose variables need locations
>> assigned
>>   * \param constants     Driver specific constant values for the program.
>>   * \param target_index  Selector for the program target to receive
>> location
>> @@ -2577,7 +2578,8 @@ find_available_slots(unsigned used_mask, unsigned
>> needed_count)
>>   * error is emitted to the shader link log and false is returned.
>>   */
>>  bool
>> -assign_attribute_or_color_locations(gl_shader_program *prog,
>> +assign_attribute_or_color_locations(void *mem_ctx,
>> +                                    gl_shader_program *prog,
>>                                      struct gl_constants *constants,
>>                                      unsigned target_index)
>>  {
>> @@ -2680,16 +2682,31 @@
>> assign_attribute_or_color_locations(gl_shader_program *prog,
>>        } else if (target_index == MESA_SHADER_FRAGMENT) {
>>          unsigned binding;
>>          unsigned index;
>> +         const char *name = var->name;
>> +         const glsl_type *type = var->type;
>> +
>> +         while (type) {
>> +            /* Check if there's a binding for the variable name */
>> +            if (prog->FragDataBindings->get(binding, name)) {
>> +               assert(binding >= FRAG_RESULT_DATA0);
>> +               var->data.location = binding;
>> +               var->data.is_unmatched_generic_inout = 0;
>> +
>> +               if (prog->FragDataIndexBindings->get(index, name)) {
>> +                  var->data.index = index;
>> +               }
>> +               break;
>> +            }
>>
>> -        if (prog->FragDataBindings->get(binding, var->name)) {
>> -           assert(binding >= FRAG_RESULT_DATA0);
>> -           var->data.location = binding;
>> -            var->data.is_unmatched_generic_inout = 0;
>> +            /* If not, but it's an array type, look for name[0] */
>> +            if (type->is_array()) {
>> +               name = ralloc_asprintf(mem_ctx, "%s[0]", name);
>> +               type = type->fields.array;
>> +               continue;
>> +            }
>>
>> -           if (prog->FragDataIndexBindings->get(index, var->name)) {
>> -              var->data.index = index;
>> -           }
>> -        }
>> +            break;
>> +         }
>>        }
>>
>>        /* From GL4.5 core spec, section 15.2 (Shader Execution):
>> @@ -4816,12 +4833,12 @@ link_shaders(struct gl_context *ctx, struct
>> gl_shader_program *prog)
>>        prev = i;
>>     }
>>
>> -   if (!assign_attribute_or_color_locations(prog, &ctx->Const,
>> +   if (!assign_attribute_or_color_locations(mem_ctx, prog, &ctx->Const,
>>                                              MESA_SHADER_VERTEX)) {
>>        goto done;
>>     }
>>
>> -   if (!assign_attribute_or_color_locations(prog, &ctx->Const,
>> +   if (!assign_attribute_or_color_locations(mem_ctx, prog, &ctx->Const,
>>                                              MESA_SHADER_FRAGMENT)) {
>>        goto done;
>>     }
>> --
>> 2.7.3
>>
>