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

[Ian Romanick]

This seems okay... let me run it through our CI first.  I should have
results later today.  I'm going to step out for a bit.

On 08/09/2016 05:43 PM, Ilia Mirkin wrote:
> 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
>>>>
>>>