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

[Ian Romanick]

Or not. :(  The patch no longer applies, and I wasn't able to trivially
make it apply.  I suspect one of Timothy's recent patches is to blame.

On 08/11/2016 10:06 AM, Ian Romanick wrote:
> 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
>>>>>
>>>>
>