[Mesa-dev] [PATCH 13/19] linker: Modify cross_validate_outputs_to_inputs to match using explicit locations
Cody Northrop
cody at lunarg.com
Wed Apr 2 16:47:30 PDT 2014
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
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> wrote:
> On Fri, Mar 28, 2014 at 5:40 AM, Ian Romanick <idr at freedesktop.org> wrote:
> > From: Ian Romanick <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>
> > ---
> > 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
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> --
> olv at LunarG.com
> _______________________________________________
> mesa-dev mailing list
> 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
Website: http://www.lunarg.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140402/1dc302a1/attachment-0001.html>
More information about the mesa-dev
mailing list