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

[Ilia Mirkin]

Looks like it all checks out. Someone refactored a bunch of the code,
and it just had to move a tiny bit, hence the conflicts. Updated patch
here:

https://github.com/imirkin/mesa/commit/56bae8a1c06aaf3b5be408ad76708de48c482909.patch

which is basically identical to the original.

On Thu, Aug 11, 2016 at 1:44 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> I'll rebase and feed it through Jenkins (I was given a branch of late).
>
> On Thu, Aug 11, 2016 at 1:38 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> 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
>>>>>>>
>>>>>>
>>>
>>