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

[Ilia Mirkin]

ping? do we want this? should i drop it?

On Wed, Jul 13, 2016 at 3:37 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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
>>>
>>