<div dir="ltr">I applied a fix locally for the problem Olv pointed out and tested the patches.<div> </div><div>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?</div> <div> 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.</div><div> </div><div>Thanks,</div><div> </div><div> -C <div>diff --git a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c</div><div>index d95d7b8..93e3fd9 100644</div> <div>--- a/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c</div><div>+++ b/tests/spec/arb_separate_shader_objects/GetProgramPipelineiv.c</div> <div>@@ -145,9 +145,12 @@ piglit_init(int argc, char **argv)</div><div> "\n"</div><div> "layout(triangles) in;\n"</div> <div> "layout(triangle_strip, max_vertices = 3) out;\n"</div><div>+ "layout(location = 0) in vec4 foo[3];\n"</div> <div>+ "layout(location = 0) out vec4 bar[3];\n"</div><div> "void main() {\n"</div><div> " for(int i = 0; i < gl_in.length(); i++) {\n"</div><div>- " gl_Position = gl_in[i].gl_Position;\n"</div> <div>+ " gl_Position = gl_in[i].gl_Position + foo[1];\n"</div><div>+ " bar[2] = foo[2];\n"</div> <div> " EmitVertex();\n"</div><div> " }\n"</div><div> " EndPrimitive();\n"</div> </div><div> </div></div><div class="gmail_extra"> <div class="gmail_quote">On Fri, Mar 28, 2014 at 12:36 AM, Chia-I Wu <<a href="mailto:olvaffe@gmail.com" target="_blank">olvaffe@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Mar 28, 2014 at 5:40 AM, Ian Romanick <<a href="mailto:idr@freedesktop.org">idr@freedesktop.org</a>> wrote: > From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> > > 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 <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> > --- > 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); </div></div>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. <div class="HOEnZb"><div class="h5"> > > /* 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div>-- olv@LunarG.com <div class="HOEnZb"><div class="h5">_______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </div></div></blockquote></div> <div> </div>-- <div dir="ltr"><div> Cody Northrop Graphics Software Engineer LunarG, Inc.- 3D Driver Innovations Email: <a href="mailto:cody@lunarg.com" target="_blank">cody@lunarg.com</a> Website: <a href="http://www.lunarg.com/" target="_blank">http://www.lunarg.com</a></div></div> </div>