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 <imir...@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 <imir...@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 > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
