<div dir="ltr">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.</div><div class="gmail_extra"> <div class="gmail_quote">On Wed, Jul 6, 2016 at 7:40 PM, Ilia Mirkin <<a href="mailto:imirkin@alum.mit.edu" target="_blank">imirkin@alum.mit.edu</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The GL spec is very unclear on this point. Apparently this is discussed without resolution in the closed Khronos bugtracker at <a href="https://cvs.khronos.org/bugzilla/show_bug.cgi?id=7829" rel="noreferrer" target="_blank">https://cvs.khronos.org/bugzilla/show_bug.cgi?id=7829</a> . 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: <a href="https://bugs.freedesktop.org/show_bug.cgi?id=96765" rel="noreferrer" target="_blank">https://bugs.freedesktop.org/show_bug.cgi?id=96765</a> Signed-off-by: Ilia Mirkin <<a href="mailto:imirkin@alum.mit.edu">imirkin@alum.mit.edu</a>> --- 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 </blockquote></div> </div>