[Mesa-dev] [PATCH 13/19] linker: Modify cross_validate_outputs_to_inputs to match using explicit locations
Ian Romanick
idr at freedesktop.org
Fri Apr 18 13:53:51 PDT 2014
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 at gmail.com
> <mailto:olvaffe at gmail.com>> wrote:
>
> On Fri, Mar 28, 2014 at 5:40 AM, Ian Romanick <idr at freedesktop.org
> <mailto:idr at freedesktop.org>> wrote:
> > From: Ian Romanick <ian.d.romanick at intel.com
> <mailto:ian.d.romanick at 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 at intel.com
> <mailto:ian.d.romanick at 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 at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> --
> olv at LunarG.com
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org <mailto:mesa-dev at lists.freedesktop.org>
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
>
> --
> Cody Northrop
> Graphics Software Engineer
> LunarG, Inc.- 3D Driver Innovations
> Email: cody at lunarg.com <mailto:cody at lunarg.com>
> Website: http://www.lunarg.com <http://www.lunarg.com/>
More information about the mesa-dev
mailing list