[Mesa-dev,13/19] linker: Modify cross_validate_outputs_to_inputs to match using explicit locations

Submitted by Cody Northrop on April 2, 2014, 11:47 p.m.

Details

Message ID CAP90KKfq_DyfuOnDwywJBZE_LK6G9+MaZuRoa9742aHudSCxaA@mail.gmail.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Cody Northrop April 2, 2014, 11:47 p.m.
I applied a fix locally for the problem Olv pointed out and tested the
patches.

Running into a problem with the linker code when a geometry shader has
location layout qualifiers for both inputs and outputs.

It falls into the assign_varying_locations() scenario that has a producer
but no consumer, which prevents the input locations from being processed.
This leads to backend instructions like these, which have negative
attribute registers:

    3: mov vgrf11.0.xyz:F, attr-1.59.xyzx:F
    4: mov vgrf12.0.xyz:F, attr-1.118.xyzx:F

Those trigger the following debug assert when a negative register indexes
the attribute_map:

    int grf = attribute_map[inst->src[i].reg + inst->src[i].reg_offset];

    /* All attributes used in the shader need to have been assigned a
    * hardware register by the caller
    */
    assert(grf != 0);

Does assign_varying_locations() need to be updated to handle when a single
shader is both a consumer and producer?  Or maybe call it twice on the same
separate shader, once as producer, once as consumer?

The below patch to an existing piglit test demonstrates the assert,
although it doesn't render to verify the locations get matched up
correctly.  Adding a geometry shader to rendezvous_by_location.c is a
better long term test, unless it is already tested somewhere I haven't
found.

Thanks,

-C




On Fri, Mar 28, 2014 at 12:36 AM, Chia-I Wu <olvaffe@gmail.com> wrote:

> On Fri, Mar 28, 2014 at 5:40 AM, Ian Romanick <idr@freedesktop.org> wrote:
> > From: Ian Romanick <ian.d.romanick@intel.com>
> >
> > This will be used for GL_ARB_separate_shader_objects.  That extension
> > not only allows separable shaders to rendezvous by location, but it also
> > allows traditionally linked shaders to rendezvous by location.  The spec
> > says:
> >
> >     36. How does the behavior of input/output interface matching differ
> >         between separable programs and non-separable programs?
> >
> >         RESOLVED: The rules for matching individual variables or block
> >         members between stages are identical for separable and
> >         non-separable programs, with one exception -- matching variables
> >         of different type with the same location, as discussed in issue
> >         34, applies only to separable programs.
> >
> >         However, the ability to enforce matching requirements differs
> >         between program types.  In non-separable programs, both sides of
> >         an interface are contained in the same linked program.  In this
> >         case, if the linker detects a mismatch, it will generate a link
> >         error.
> >
> > Signed-off-by: Ian Romanick <ian.d.romanick@intel.com>
> > ---
> >  src/glsl/link_varyings.cpp       | 73
> +++++++++++++++++++++++++++++++++++-----
> >  src/glsl/tests/varyings_test.cpp | 35 ++++++++++++-------
> >  2 files changed, 88 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
> > index 3d9516c..3f80dbd 100644
> > --- a/src/glsl/link_varyings.cpp
> > +++ b/src/glsl/link_varyings.cpp
> > @@ -172,6 +172,7 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
> >                                  gl_shader *producer, gl_shader
> *consumer)
> >  {
> >     glsl_symbol_table parameters;
> > +   ir_variable *explicit_locations[MAX_VARYING] = { NULL, };
> >
> >     /* Find all shader outputs in the "producer" stage.
> >      */
> > @@ -181,7 +182,26 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
> >        if ((var == NULL) || (var->data.mode != ir_var_shader_out))
> >          continue;
> >
> > -      parameters.add_variable(var);
> > +      if (!var->data.explicit_location
> > +          || var->data.location < VARYING_SLOT_VAR0)
> > +         parameters.add_variable(var);
> > +      else {
> > +         /* User-defined varyings with explicit locations are handled
> > +          * differently because they do not need to have matching names.
> > +          */
> > +         const unsigned idx = var->data.location - VARYING_SLOT_VAR0;
> > +
> > +         if (explicit_locations[idx] != NULL) {
> > +            linker_error(prog,
> > +                         "%s shader has multiple outputs explicitly "
> > +                         "assigned to location %d\n",
> > +                         _mesa_shader_stage_to_string(producer->Stage),
> > +                         idx);
> > +            return;
> > +         }
> > +
> > +         explicit_locations[idx] = var;
> > +      }
> >     }
> >
> >
> > @@ -220,7 +240,27 @@ cross_validate_outputs_to_inputs(struct
> gl_shader_program *prog,
> >                                               front_color, back_color,
> >                                               consumer->Stage,
> producer->Stage);
> >        } else {
> > -         ir_variable *const output =
> parameters.get_variable(input->name);
> > +         /* The rules for connecting inputs and outputs change in the
> presence
> > +          * of explicit locations.  In this case, we no longer care
> about the
> > +          * names of the variables.  Instead, we care only about the
> > +          * explicitly assigned location.
> > +          */
> > +         ir_variable *output = NULL;
> > +         if (input->data.explicit_location
> > +             && input->data.location >= VARYING_SLOT_VAR0) {
> > +            output = explicit_locations[input->data.location -
> VARYING_SLOT_VAR0];
> > +
> > +            if (output == NULL) {
> > +               linker_error(prog,
> > +                            "%s shader input `%s' with explicit
> location "
> > +                            "has no matching output\n",
> > +
>  _mesa_shader_stage_to_string(consumer->Stage),
> > +                            input->name);
> > +            }
> > +         } else {
> > +            output = parameters.get_variable(input->name);
> > +         }
> > +
> >           if (output != NULL) {
> >              cross_validate_types_and_qualifiers(prog, input, output,
> >                                                  consumer->Stage,
> producer->Stage);
> > @@ -1052,8 +1092,13 @@ namespace linker {
> >  bool
> >  populate_consumer_input_sets(void *mem_ctx, exec_list *ir,
> >                               hash_table *consumer_inputs,
> > -                             hash_table *consumer_interface_inputs)
> > +                             hash_table *consumer_interface_inputs,
> > +                             ir_variable
> *consumer_inputs_with_locations[MAX_VARYING])
> >  {
> > +   memset(consumer_inputs_with_locations,
> > +          0,
> > +          sizeof(consumer_inputs_with_locations[0]) * MAX_VARYING);
> > +
> >     foreach_list(node, ir) {
> >        ir_variable *const input_var = ((ir_instruction *)
> node)->as_variable();
> >
> > @@ -1061,7 +1106,13 @@ populate_consumer_input_sets(void *mem_ctx,
> exec_list *ir,
> >           if (input_var->type->is_interface())
> >              return false;
> >
> > -         if (input_var->get_interface_type() != NULL) {
> > +         if (input_var->data.user_location != -1) {
> > +            /* FINISHME: If the input is an interface, array, or
> structure, it
> > +             * FINISHME: will use multiple slots.
> > +             */
> > +
>  consumer_inputs_with_locations[input_var->data.user_location] =
> > +               input_var;
> > +         } else if (input_var->get_interface_type() != NULL) {
> >              char *const iface_field_name =
> >                 ralloc_asprintf(mem_ctx, "%s.%s",
> >                                 input_var->get_interface_type()->name,
> > @@ -1088,11 +1139,14 @@ ir_variable *
> >  get_matching_input(void *mem_ctx,
> >                     const ir_variable *output_var,
> >                     hash_table *consumer_inputs,
> > -                   hash_table *consumer_interface_inputs)
> > +                   hash_table *consumer_interface_inputs,
> > +                   ir_variable
> *consumer_inputs_with_locations[MAX_VARYING])
> >  {
> >     ir_variable *input_var;
> >
> > -   if (output_var->get_interface_type() != NULL) {
> > +   if (output_var->data.user_location != -1) {
> > +      input_var =
> consumer_inputs_with_locations[output_var->data.user_location];
> > +   } else if (output_var->get_interface_type() != NULL) {
> >        char *const iface_field_name =
> >           ralloc_asprintf(mem_ctx, "%s.%s",
> >                           output_var->get_interface_type()->name,
> > @@ -1213,6 +1267,7 @@ assign_varying_locations(struct gl_context *ctx,
> >        = hash_table_ctor(0, hash_table_string_hash,
> hash_table_string_compare);
> >     hash_table *consumer_interface_inputs
> >        = hash_table_ctor(0, hash_table_string_hash,
> hash_table_string_compare);
> > +   ir_variable *consumer_inputs_with_locations[MAX_VARYING];
> >
> >     /* Operate in a total of four passes.
> >      *
> > @@ -1239,7 +1294,8 @@ assign_varying_locations(struct gl_context *ctx,
> >         && !linker::populate_consumer_input_sets(mem_ctx,
> >                                                  consumer->ir,
> >                                                  consumer_inputs,
> > -
>  consumer_interface_inputs)) {
> > +
>  consumer_interface_inputs,
> > +
>  consumer_inputs_with_locations)) {
> >        assert(!"populate_consumer_input_sets failed");
> >        hash_table_dtor(tfeedback_candidates);
> >        hash_table_dtor(consumer_inputs);
> > @@ -1261,7 +1317,8 @@ assign_varying_locations(struct gl_context *ctx,
> >
> >           ir_variable *const input_var =
> >              linker::get_matching_input(mem_ctx, output_var,
> consumer_inputs,
> > -                                       consumer_interface_inputs);
> > +                                       consumer_interface_inputs,
> > +                                       consumer_inputs_with_locations);
> consumer_inputs_with_locations is uninitialized when there is no
> consumer.  get_matching_input would return garbage when output_var has
> a user location.
>
> Something I ran into when testing the patches out.
>
> >
> >           /* If a matching input variable was found, add this ouptut
> (and the
> >            * input) to the set.  If this is a separable program and
> there is no
> > diff --git a/src/glsl/tests/varyings_test.cpp
> b/src/glsl/tests/varyings_test.cpp
> > index 1749112..8a188a7 100644
> > --- a/src/glsl/tests/varyings_test.cpp
> > +++ b/src/glsl/tests/varyings_test.cpp
> > @@ -38,13 +38,15 @@ namespace linker {
> >  bool
> >  populate_consumer_input_sets(void *mem_ctx, exec_list *ir,
> >                               hash_table *consumer_inputs,
> > -                             hash_table *consumer_interface_inputs);
> > +                             hash_table *consumer_interface_inputs,
> > +                             ir_variable
> *consumer_inputs_with_locations[MAX_VARYING]);
> >
> >  ir_variable *
> >  get_matching_input(void *mem_ctx,
> >                     const ir_variable *output_var,
> >                     hash_table *consumer_inputs,
> > -                   hash_table *consumer_interface_inputs);
> > +                   hash_table *consumer_interface_inputs,
> > +                   ir_variable
> *consumer_inputs_with_locations[MAX_VARYING]);
> >  }
> >
> >  class link_varyings : public ::testing::Test {
> > @@ -68,6 +70,7 @@ public:
> >     hash_table *consumer_interface_inputs;
> >
> >     const glsl_type *simple_interface;
> > +   ir_variable *junk[MAX_VARYING];
> >  };
> >
> >  link_varyings::link_varyings()
> > @@ -164,7 +167,8 @@ TEST_F(link_varyings, single_simple_input)
> >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
> >                                                      &ir,
> >                                                      consumer_inputs,
> > -
>  consumer_interface_inputs));
> > +
>  consumer_interface_inputs,
> > +                                                    junk));
> >
> >     EXPECT_EQ((void *) v, hash_table_find(consumer_inputs, "a"));
> >     EXPECT_EQ(1u, num_elements(consumer_inputs));
> > @@ -190,7 +194,8 @@ TEST_F(link_varyings, gl_ClipDistance)
> >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
> >                                                      &ir,
> >                                                      consumer_inputs,
> > -
>  consumer_interface_inputs));
> > +
>  consumer_interface_inputs,
> > +                                                    junk));
> >
> >     EXPECT_EQ((void *) clipdistance,
> >               hash_table_find(consumer_inputs, "gl_ClipDistance"));
> > @@ -212,8 +217,8 @@ TEST_F(link_varyings, single_interface_input)
> >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
> >                                                      &ir,
> >                                                      consumer_inputs,
> > -
>  consumer_interface_inputs));
> > -
> > +
>  consumer_interface_inputs,
> > +                                                    junk));
> >     char *const full_name = interface_field_name(simple_interface);
> >
> >     EXPECT_EQ((void *) v, hash_table_find(consumer_interface_inputs,
> full_name));
> > @@ -243,7 +248,8 @@ TEST_F(link_varyings,
> one_interface_and_one_simple_input)
> >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
> >                                                      &ir,
> >                                                      consumer_inputs,
> > -
>  consumer_interface_inputs));
> > +
>  consumer_interface_inputs,
> > +                                                    junk));
> >
> >     char *const iface_field_name =
> interface_field_name(simple_interface);
> >
> > @@ -269,7 +275,8 @@ TEST_F(link_varyings, invalid_interface_input)
> >     EXPECT_FALSE(linker::populate_consumer_input_sets(mem_ctx,
> >                                                      &ir,
> >                                                      consumer_inputs,
> > -
>  consumer_interface_inputs));
> > +
> consumer_interface_inputs,
> > +                                                     junk));
> >  }
> >
> >  TEST_F(link_varyings, interface_field_doesnt_match_noninterface)
> > @@ -288,7 +295,8 @@ TEST_F(link_varyings,
> interface_field_doesnt_match_noninterface)
> >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
> >                                                      &ir,
> >                                                      consumer_inputs,
> > -
>  consumer_interface_inputs));
> > +
>  consumer_interface_inputs,
> > +                                                    junk));
> >
> >     /* Create an output variable, "v", that is part of an interface
> block named
> >      * "a".  They should not match.
> > @@ -304,7 +312,8 @@ TEST_F(link_varyings,
> interface_field_doesnt_match_noninterface)
> >        linker::get_matching_input(mem_ctx,
> >                                   out_v,
> >                                   consumer_inputs,
> > -                                 consumer_interface_inputs);
> > +                                 consumer_interface_inputs,
> > +                                 junk);
> >
> >     EXPECT_EQ(NULL, match);
> >  }
> > @@ -328,7 +337,8 @@ TEST_F(link_varyings,
> interface_field_doesnt_match_noninterface_vice_versa)
> >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
> >                                                      &ir,
> >                                                      consumer_inputs,
> > -
>  consumer_interface_inputs));
> > +
>  consumer_interface_inputs,
> > +                                                    junk));
> >
> >     /* Create an output variable "a.v".  They should not match.
> >      */
> > @@ -341,7 +351,8 @@ TEST_F(link_varyings,
> interface_field_doesnt_match_noninterface_vice_versa)
> >        linker::get_matching_input(mem_ctx,
> >                                   out_v,
> >                                   consumer_inputs,
> > -                                 consumer_interface_inputs);
> > +                                 consumer_interface_inputs,
> > +                                 junk);
> >
> >     EXPECT_EQ(NULL, match);
> >  }
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> --
> olv@LunarG.com
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

Patch hide | download patch | download mbox

diff --git a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
index d95d7b8..93e3fd9 100644
--- a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
+++ b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
@@ -145,9 +145,12 @@  piglit_init(int argc, char **argv)
                "\n"
                "layout(triangles) in;\n"
                "layout(triangle_strip, max_vertices = 3) out;\n"
+               "layout(location = 0) in vec4 foo[3];\n"
+               "layout(location = 0) out vec4 bar[3];\n"
                "void main() {\n"
                "    for(int i = 0; i < gl_in.length(); i++) {\n"
-               "        gl_Position = gl_in[i].gl_Position;\n"
+               "        gl_Position = gl_in[i].gl_Position + foo[1];\n"
+               "        bar[2] = foo[2];\n"
                "        EmitVertex();\n"
                "    }\n"
                "    EndPrimitive();\n"

Comments

On 04/02/2014 04:47 PM, Cody Northrop wrote:
> I applied a fix locally for the problem Olv pointed out and tested the
> patches.
> 
> Running into a problem with the linker code when a geometry shader has
> location layout qualifiers for both inputs and outputs.
> 
> It falls into the assign_varying_locations() scenario that has a
> producer but no consumer, which prevents the input locations from being
> processed.  This leads to backend instructions like these, which have
> negative attribute registers:
> 
>     3: mov vgrf11.0.xyz:F, attr-1.59.xyzx:F
>     4: mov vgrf12.0.xyz:F, attr-1.118.xyzx:F
> 
> Those trigger the following debug assert when a negative register
> indexes the attribute_map:
> 
>     int grf = attribute_map[inst->src[i].reg + inst->src[i].reg_offset];
> 
>     /* All attributes used in the shader need to have been assigned a
>     * hardware register by the caller
>     */
>     assert(grf != 0);
> 
> Does assign_varying_locations() need to be updated to handle when a
> single shader is both a consumer and producer?  Or maybe call it twice
> on the same separate shader, once as producer, once as consumer?

In the non-separable case, the geometry shader is processed twice: once
to connect it with the vertex shader and once to connect it with the
fragment shader.  My instinct tells me that the separable case should
follow the same pattern.

> The below patch to an existing piglit test demonstrates the assert,
> although it doesn't render to verify the locations get matched up
> correctly.  Adding a geometry shader to rendezvous_by_location.c is a
> better long term test, unless it is already tested somewhere I haven't
> found.

There is woefully little testing of geometry shaders with SSO.  Eric
pointed out another missing test in his review of patch 10. :(

> Thanks,
> 
> -C
> 
> diff --git
> a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
> b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
> index d95d7b8..93e3fd9 100644
> --- a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
> +++ b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c
> @@ -145,9 +145,12 @@ piglit_init(int argc, char **argv)
>                 "\n"
>                 "layout(triangles) in;\n"
>                 "layout(triangle_strip, max_vertices = 3) out;\n"
> +               "layout(location = 0) in vec4 foo[3];\n"
> +               "layout(location = 0) out vec4 bar[3];\n"
>                 "void main() {\n"
>                 "    for(int i = 0; i < gl_in.length(); i++) {\n"
> -               "        gl_Position = gl_in[i].gl_Position;\n"
> +               "        gl_Position = gl_in[i].gl_Position + foo[1];\n"
> +               "        bar[2] = foo[2];\n"
>                 "        EmitVertex();\n"
>                 "    }\n"
>                 "    EndPrimitive();\n"
> 
> 
> 
> On Fri, Mar 28, 2014 at 12:36 AM, Chia-I Wu <olvaffe@gmail.com
> <mailto:olvaffe@gmail.com>> wrote:
> 
>     On Fri, Mar 28, 2014 at 5:40 AM, Ian Romanick <idr@freedesktop.org
>     <mailto:idr@freedesktop.org>> wrote:
>     > From: Ian Romanick <ian.d.romanick@intel.com
>     <mailto:ian.d.romanick@intel.com>>
>     >
>     > This will be used for GL_ARB_separate_shader_objects.  That extension
>     > not only allows separable shaders to rendezvous by location, but
>     it also
>     > allows traditionally linked shaders to rendezvous by location.
>      The spec
>     > says:
>     >
>     >     36. How does the behavior of input/output interface matching
>     differ
>     >         between separable programs and non-separable programs?
>     >
>     >         RESOLVED: The rules for matching individual variables or block
>     >         members between stages are identical for separable and
>     >         non-separable programs, with one exception -- matching
>     variables
>     >         of different type with the same location, as discussed in
>     issue
>     >         34, applies only to separable programs.
>     >
>     >         However, the ability to enforce matching requirements differs
>     >         between program types.  In non-separable programs, both
>     sides of
>     >         an interface are contained in the same linked program.  In
>     this
>     >         case, if the linker detects a mismatch, it will generate a
>     link
>     >         error.
>     >
>     > Signed-off-by: Ian Romanick <ian.d.romanick@intel.com
>     <mailto:ian.d.romanick@intel.com>>
>     > ---
>     >  src/glsl/link_varyings.cpp       | 73
>     +++++++++++++++++++++++++++++++++++-----
>     >  src/glsl/tests/varyings_test.cpp | 35 ++++++++++++-------
>     >  2 files changed, 88 insertions(+), 20 deletions(-)
>     >
>     > diff --git a/src/glsl/link_varyings.cpp b/src/glsl/link_varyings.cpp
>     > index 3d9516c..3f80dbd 100644
>     > --- a/src/glsl/link_varyings.cpp
>     > +++ b/src/glsl/link_varyings.cpp
>     > @@ -172,6 +172,7 @@ cross_validate_outputs_to_inputs(struct
>     gl_shader_program *prog,
>     >                                  gl_shader *producer, gl_shader
>     *consumer)
>     >  {
>     >     glsl_symbol_table parameters;
>     > +   ir_variable *explicit_locations[MAX_VARYING] = { NULL, };
>     >
>     >     /* Find all shader outputs in the "producer" stage.
>     >      */
>     > @@ -181,7 +182,26 @@ cross_validate_outputs_to_inputs(struct
>     gl_shader_program *prog,
>     >        if ((var == NULL) || (var->data.mode != ir_var_shader_out))
>     >          continue;
>     >
>     > -      parameters.add_variable(var);
>     > +      if (!var->data.explicit_location
>     > +          || var->data.location < VARYING_SLOT_VAR0)
>     > +         parameters.add_variable(var);
>     > +      else {
>     > +         /* User-defined varyings with explicit locations are handled
>     > +          * differently because they do not need to have matching
>     names.
>     > +          */
>     > +         const unsigned idx = var->data.location - VARYING_SLOT_VAR0;
>     > +
>     > +         if (explicit_locations[idx] != NULL) {
>     > +            linker_error(prog,
>     > +                         "%s shader has multiple outputs explicitly "
>     > +                         "assigned to location %d\n",
>     > +                        
>     _mesa_shader_stage_to_string(producer->Stage),
>     > +                         idx);
>     > +            return;
>     > +         }
>     > +
>     > +         explicit_locations[idx] = var;
>     > +      }
>     >     }
>     >
>     >
>     > @@ -220,7 +240,27 @@ cross_validate_outputs_to_inputs(struct
>     gl_shader_program *prog,
>     >                                               front_color, back_color,
>     >                                               consumer->Stage,
>     producer->Stage);
>     >        } else {
>     > -         ir_variable *const output =
>     parameters.get_variable(input->name);
>     > +         /* The rules for connecting inputs and outputs change in
>     the presence
>     > +          * of explicit locations.  In this case, we no longer
>     care about the
>     > +          * names of the variables.  Instead, we care only about the
>     > +          * explicitly assigned location.
>     > +          */
>     > +         ir_variable *output = NULL;
>     > +         if (input->data.explicit_location
>     > +             && input->data.location >= VARYING_SLOT_VAR0) {
>     > +            output = explicit_locations[input->data.location -
>     VARYING_SLOT_VAR0];
>     > +
>     > +            if (output == NULL) {
>     > +               linker_error(prog,
>     > +                            "%s shader input `%s' with explicit
>     location "
>     > +                            "has no matching output\n",
>     > +                          
>      _mesa_shader_stage_to_string(consumer->Stage),
>     > +                            input->name);
>     > +            }
>     > +         } else {
>     > +            output = parameters.get_variable(input->name);
>     > +         }
>     > +
>     >           if (output != NULL) {
>     >              cross_validate_types_and_qualifiers(prog, input, output,
>     >                                                  consumer->Stage,
>     producer->Stage);
>     > @@ -1052,8 +1092,13 @@ namespace linker {
>     >  bool
>     >  populate_consumer_input_sets(void *mem_ctx, exec_list *ir,
>     >                               hash_table *consumer_inputs,
>     > -                             hash_table *consumer_interface_inputs)
>     > +                             hash_table *consumer_interface_inputs,
>     > +                             ir_variable
>     *consumer_inputs_with_locations[MAX_VARYING])
>     >  {
>     > +   memset(consumer_inputs_with_locations,
>     > +          0,
>     > +          sizeof(consumer_inputs_with_locations[0]) * MAX_VARYING);
>     > +
>     >     foreach_list(node, ir) {
>     >        ir_variable *const input_var = ((ir_instruction *)
>     node)->as_variable();
>     >
>     > @@ -1061,7 +1106,13 @@ populate_consumer_input_sets(void *mem_ctx,
>     exec_list *ir,
>     >           if (input_var->type->is_interface())
>     >              return false;
>     >
>     > -         if (input_var->get_interface_type() != NULL) {
>     > +         if (input_var->data.user_location != -1) {
>     > +            /* FINISHME: If the input is an interface, array, or
>     structure, it
>     > +             * FINISHME: will use multiple slots.
>     > +             */
>     > +          
>      consumer_inputs_with_locations[input_var->data.user_location] =
>     > +               input_var;
>     > +         } else if (input_var->get_interface_type() != NULL) {
>     >              char *const iface_field_name =
>     >                 ralloc_asprintf(mem_ctx, "%s.%s",
>     >                                 input_var->get_interface_type()->name,
>     > @@ -1088,11 +1139,14 @@ ir_variable *
>     >  get_matching_input(void *mem_ctx,
>     >                     const ir_variable *output_var,
>     >                     hash_table *consumer_inputs,
>     > -                   hash_table *consumer_interface_inputs)
>     > +                   hash_table *consumer_interface_inputs,
>     > +                   ir_variable
>     *consumer_inputs_with_locations[MAX_VARYING])
>     >  {
>     >     ir_variable *input_var;
>     >
>     > -   if (output_var->get_interface_type() != NULL) {
>     > +   if (output_var->data.user_location != -1) {
>     > +      input_var =
>     consumer_inputs_with_locations[output_var->data.user_location];
>     > +   } else if (output_var->get_interface_type() != NULL) {
>     >        char *const iface_field_name =
>     >           ralloc_asprintf(mem_ctx, "%s.%s",
>     >                           output_var->get_interface_type()->name,
>     > @@ -1213,6 +1267,7 @@ assign_varying_locations(struct gl_context *ctx,
>     >        = hash_table_ctor(0, hash_table_string_hash,
>     hash_table_string_compare);
>     >     hash_table *consumer_interface_inputs
>     >        = hash_table_ctor(0, hash_table_string_hash,
>     hash_table_string_compare);
>     > +   ir_variable *consumer_inputs_with_locations[MAX_VARYING];
>     >
>     >     /* Operate in a total of four passes.
>     >      *
>     > @@ -1239,7 +1294,8 @@ assign_varying_locations(struct gl_context *ctx,
>     >         && !linker::populate_consumer_input_sets(mem_ctx,
>     >                                                  consumer->ir,
>     >                                                  consumer_inputs,
>     > -                                              
>      consumer_interface_inputs)) {
>     > +                                              
>      consumer_interface_inputs,
>     > +                                              
>      consumer_inputs_with_locations)) {
>     >        assert(!"populate_consumer_input_sets failed");
>     >        hash_table_dtor(tfeedback_candidates);
>     >        hash_table_dtor(consumer_inputs);
>     > @@ -1261,7 +1317,8 @@ assign_varying_locations(struct gl_context *ctx,
>     >
>     >           ir_variable *const input_var =
>     >              linker::get_matching_input(mem_ctx, output_var,
>     consumer_inputs,
>     > -                                       consumer_interface_inputs);
>     > +                                       consumer_interface_inputs,
>     > +                                      
>     consumer_inputs_with_locations);
>     consumer_inputs_with_locations is uninitialized when there is no
>     consumer.  get_matching_input would return garbage when output_var has
>     a user location.
> 
>     Something I ran into when testing the patches out.
> 
>     >
>     >           /* If a matching input variable was found, add this
>     ouptut (and the
>     >            * input) to the set.  If this is a separable program
>     and there is no
>     > diff --git a/src/glsl/tests/varyings_test.cpp
>     b/src/glsl/tests/varyings_test.cpp
>     > index 1749112..8a188a7 100644
>     > --- a/src/glsl/tests/varyings_test.cpp
>     > +++ b/src/glsl/tests/varyings_test.cpp
>     > @@ -38,13 +38,15 @@ namespace linker {
>     >  bool
>     >  populate_consumer_input_sets(void *mem_ctx, exec_list *ir,
>     >                               hash_table *consumer_inputs,
>     > -                             hash_table *consumer_interface_inputs);
>     > +                             hash_table *consumer_interface_inputs,
>     > +                             ir_variable
>     *consumer_inputs_with_locations[MAX_VARYING]);
>     >
>     >  ir_variable *
>     >  get_matching_input(void *mem_ctx,
>     >                     const ir_variable *output_var,
>     >                     hash_table *consumer_inputs,
>     > -                   hash_table *consumer_interface_inputs);
>     > +                   hash_table *consumer_interface_inputs,
>     > +                   ir_variable
>     *consumer_inputs_with_locations[MAX_VARYING]);
>     >  }
>     >
>     >  class link_varyings : public ::testing::Test {
>     > @@ -68,6 +70,7 @@ public:
>     >     hash_table *consumer_interface_inputs;
>     >
>     >     const glsl_type *simple_interface;
>     > +   ir_variable *junk[MAX_VARYING];
>     >  };
>     >
>     >  link_varyings::link_varyings()
>     > @@ -164,7 +167,8 @@ TEST_F(link_varyings, single_simple_input)
>     >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
>     >                                                      &ir,
>     >                                                      consumer_inputs,
>     > -                                                  
>      consumer_interface_inputs));
>     > +                                                  
>      consumer_interface_inputs,
>     > +                                                    junk));
>     >
>     >     EXPECT_EQ((void *) v, hash_table_find(consumer_inputs, "a"));
>     >     EXPECT_EQ(1u, num_elements(consumer_inputs));
>     > @@ -190,7 +194,8 @@ TEST_F(link_varyings, gl_ClipDistance)
>     >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
>     >                                                      &ir,
>     >                                                      consumer_inputs,
>     > -                                                  
>      consumer_interface_inputs));
>     > +                                                  
>      consumer_interface_inputs,
>     > +                                                    junk));
>     >
>     >     EXPECT_EQ((void *) clipdistance,
>     >               hash_table_find(consumer_inputs, "gl_ClipDistance"));
>     > @@ -212,8 +217,8 @@ TEST_F(link_varyings, single_interface_input)
>     >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
>     >                                                      &ir,
>     >                                                      consumer_inputs,
>     > -                                                  
>      consumer_interface_inputs));
>     > -
>     > +                                                  
>      consumer_interface_inputs,
>     > +                                                    junk));
>     >     char *const full_name = interface_field_name(simple_interface);
>     >
>     >     EXPECT_EQ((void *) v,
>     hash_table_find(consumer_interface_inputs, full_name));
>     > @@ -243,7 +248,8 @@ TEST_F(link_varyings,
>     one_interface_and_one_simple_input)
>     >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
>     >                                                      &ir,
>     >                                                      consumer_inputs,
>     > -                                                  
>      consumer_interface_inputs));
>     > +                                                  
>      consumer_interface_inputs,
>     > +                                                    junk));
>     >
>     >     char *const iface_field_name =
>     interface_field_name(simple_interface);
>     >
>     > @@ -269,7 +275,8 @@ TEST_F(link_varyings, invalid_interface_input)
>     >     EXPECT_FALSE(linker::populate_consumer_input_sets(mem_ctx,
>     >                                                      &ir,
>     >                                                      consumer_inputs,
>     > -                                                  
>      consumer_interface_inputs));
>     > +                                                    
>     consumer_interface_inputs,
>     > +                                                     junk));
>     >  }
>     >
>     >  TEST_F(link_varyings, interface_field_doesnt_match_noninterface)
>     > @@ -288,7 +295,8 @@ TEST_F(link_varyings,
>     interface_field_doesnt_match_noninterface)
>     >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
>     >                                                      &ir,
>     >                                                      consumer_inputs,
>     > -                                                  
>      consumer_interface_inputs));
>     > +                                                  
>      consumer_interface_inputs,
>     > +                                                    junk));
>     >
>     >     /* Create an output variable, "v", that is part of an
>     interface block named
>     >      * "a".  They should not match.
>     > @@ -304,7 +312,8 @@ TEST_F(link_varyings,
>     interface_field_doesnt_match_noninterface)
>     >        linker::get_matching_input(mem_ctx,
>     >                                   out_v,
>     >                                   consumer_inputs,
>     > -                                 consumer_interface_inputs);
>     > +                                 consumer_interface_inputs,
>     > +                                 junk);
>     >
>     >     EXPECT_EQ(NULL, match);
>     >  }
>     > @@ -328,7 +337,8 @@ TEST_F(link_varyings,
>     interface_field_doesnt_match_noninterface_vice_versa)
>     >     ASSERT_TRUE(linker::populate_consumer_input_sets(mem_ctx,
>     >                                                      &ir,
>     >                                                      consumer_inputs,
>     > -                                                  
>      consumer_interface_inputs));
>     > +                                                  
>      consumer_interface_inputs,
>     > +                                                    junk));
>     >
>     >     /* Create an output variable "a.v".  They should not match.
>     >      */
>     > @@ -341,7 +351,8 @@ TEST_F(link_varyings,
>     interface_field_doesnt_match_noninterface_vice_versa)
>     >        linker::get_matching_input(mem_ctx,
>     >                                   out_v,
>     >                                   consumer_inputs,
>     > -                                 consumer_interface_inputs);
>     > +                                 consumer_interface_inputs,
>     > +                                 junk);
>     >
>     >     EXPECT_EQ(NULL, match);
>     >  }
>     > --
>     > 1.8.1.4
>     >
>     > _______________________________________________
>     > mesa-dev mailing list
>     > mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
>     > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
> 
>     --
>     olv@LunarG.com
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org>
>     http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 
> 
> 
> 
> -- 
>  Cody Northrop
>  Graphics Software Engineer
>  LunarG, Inc.- 3D Driver Innovations
>  Email: cody@lunarg.com <mailto:cody@lunarg.com>
>  Website: http://www.lunarg.com <http://www.lunarg.com/>