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

[Corentin Wallez]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160708/1390b3ba/attachment.html>