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

[Ilia Mirkin]

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
>>>>>>
>>>>>
>>
>