On 10/18/2013 01:31 PM, Paul Berry wrote: > On 11 October 2013 11:18, Ian Romanick <i...@freedesktop.org > <mailto:i...@freedesktop.org>> wrote: > > From: Ian Romanick <ian.d.roman...@intel.com > <mailto:ian.d.roman...@intel.com>> > > This required fixing the out-of-date prototype in linker.h. While > making that change, further change the prototype to make unit testing a > bit easier. > > > I'd prefer to see this split into two patches: one to change the > signature of the function, and a second one to add the unit test. But I > won't be a stickler about it.
Since there are other changes to be made, that should be easy enough. > Validates: > > - ir_variable::explicit_location should not be modified. > > - If ir_variable::location is not a generic location (i.e., it's a > location for a built-in), ir_variable::location is not modified. > ir_variable::location_frac must be reset to 0. > ir_variable::is_unmatched_generic_inout must be reset to 1. > > - If ir_variable::explicit_location is not set, ir_variable::location, > ir_variable::location_frac, and > ir_variable::is_unmatched_generic_inout must be reset to 0. > > - If ir_variable::explicit_location is set, ir_variable::location > should not be modified. ir_variable::location_frac, and > ir_variable::is_unmatched_generic_inout must be reset to 0. > > Signed-off-by: Ian Romanick <ian.d.roman...@intel.com > <mailto:ian.d.roman...@intel.com>> > --- > src/glsl/Makefile.am | 1 + > src/glsl/linker.cpp | 10 +- > src/glsl/linker.h | 4 +- > src/glsl/tests/invalidate_locations_test.cpp | 266 > +++++++++++++++++++++++++++ > 4 files changed, 274 insertions(+), 7 deletions(-) > create mode 100644 src/glsl/tests/invalidate_locations_test.cpp > > diff --git a/src/glsl/Makefile.am b/src/glsl/Makefile.am > index 80949fb..b9ed5b6 100644 > --- a/src/glsl/Makefile.am > +++ b/src/glsl/Makefile.am > @@ -60,6 +60,7 @@ tests_general_ir_test_SOURCES = \ > $(top_srcdir)/src/mesa/program/symbol_table.c \ > $(GLSL_SRCDIR)/standalone_scaffolding.cpp \ > tests/builtin_variable_test.cpp \ > + tests/invalidate_locations_test.cpp \ > tests/general_ir_test.cpp > tests_general_ir_test_CFLAGS = \ > $(PTHREAD_CFLAGS) > diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp > index 9095a40..02eb4e1 100644 > --- a/src/glsl/linker.cpp > +++ b/src/glsl/linker.cpp > @@ -366,10 +366,10 @@ parse_program_resource_name(const GLchar *name, > > > void > -link_invalidate_variable_locations(gl_shader *sh, int input_base, > +link_invalidate_variable_locations(exec_list *ir, int input_base, > int output_base) > { > - foreach_list(node, sh->ir) { > + foreach_list(node, ir) { > ir_variable *const var = ((ir_instruction *) > node)->as_variable(); > > if (var == NULL) > @@ -2221,17 +2221,17 @@ link_shaders(struct gl_context *ctx, struct > gl_shader_program *prog) > /* Mark all generic shader inputs and outputs as unpaired. */ > if (prog->_LinkedShaders[MESA_SHADER_VERTEX] != NULL) { > link_invalidate_variable_locations( > - prog->_LinkedShaders[MESA_SHADER_VERTEX], > + prog->_LinkedShaders[MESA_SHADER_VERTEX]->ir, > VERT_ATTRIB_GENERIC0, VARYING_SLOT_VAR0); > } > if (prog->_LinkedShaders[MESA_SHADER_GEOMETRY] != NULL) { > link_invalidate_variable_locations( > - prog->_LinkedShaders[MESA_SHADER_GEOMETRY], > + prog->_LinkedShaders[MESA_SHADER_GEOMETRY]->ir, > VARYING_SLOT_VAR0, VARYING_SLOT_VAR0); > } > if (prog->_LinkedShaders[MESA_SHADER_FRAGMENT] != NULL) { > link_invalidate_variable_locations( > - prog->_LinkedShaders[MESA_SHADER_FRAGMENT], > + prog->_LinkedShaders[MESA_SHADER_FRAGMENT]->ir, > VARYING_SLOT_VAR0, FRAG_RESULT_DATA0); > } > > diff --git a/src/glsl/linker.h b/src/glsl/linker.h > index 8a0027d..9915c38 100644 > --- a/src/glsl/linker.h > +++ b/src/glsl/linker.h > @@ -31,8 +31,8 @@ link_function_calls(gl_shader_program *prog, > gl_shader *main, > gl_shader **shader_list, unsigned num_shaders); > > extern void > -link_invalidate_variable_locations(gl_shader *sh, enum > ir_variable_mode mode, > - int generic_base); > +link_invalidate_variable_locations(exec_list *ir, int input_base, > + int output_base); > > extern void > link_assign_uniform_locations(struct gl_shader_program *prog); > diff --git a/src/glsl/tests/invalidate_locations_test.cpp > b/src/glsl/tests/invalidate_locations_test.cpp > new file mode 100644 > index 0000000..958acec > --- /dev/null > +++ b/src/glsl/tests/invalidate_locations_test.cpp > @@ -0,0 +1,266 @@ > +/* > + * Copyright © 2013 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person > obtaining a > + * copy of this software and associated documentation files (the > "Software"), > + * to deal in the Software without restriction, including without > limitation > + * the rights to use, copy, modify, merge, publish, distribute, > sublicense, > + * and/or sell copies of the Software, and to permit persons to > whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including > the next > + * paragraph) shall be included in all copies or substantial > portions of the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO > EVENT SHALL > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, > DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, > ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + */ > +#include <gtest/gtest.h> > +#include "main/compiler.h" > +#include "main/mtypes.h" > +#include "main/macros.h" > +#include "ralloc.h" > +#include "ir.h" > +#include "linker.h" > + > +/** > + * \file varyings_test.cpp > + * > + * Test various aspects of linking shader stage inputs and outputs. > + */ > + > +class invalidate_locations : public ::testing::Test { > +public: > + virtual void SetUp(); > + virtual void TearDown(); > + > + void *mem_ctx; > + exec_list ir; > +}; > + > +void > +invalidate_locations::SetUp() > +{ > + this->mem_ctx = ralloc_context(NULL); > + this->ir.make_empty(); > +} > + > +void > +invalidate_locations::TearDown() > +{ > + ralloc_free(this->mem_ctx); > + this->mem_ctx = NULL; > +} > + > +TEST_F(invalidate_locations, simple_vertex_in_generic) > +{ > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "a", > + ir_var_shader_in); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VERT_ATTRIB_GENERIC0; > + var->location_frac = 2; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(-1, var->location); > + EXPECT_EQ(0u, var->location_frac); > + EXPECT_FALSE(var->explicit_location); > + EXPECT_TRUE(var->is_unmatched_generic_inout); > +} > + > +TEST_F(invalidate_locations, explicit_location_vertex_in_generic) > +{ > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "a", > + ir_var_shader_in); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VERT_ATTRIB_GENERIC0; > + var->explicit_location = true; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(VERT_ATTRIB_GENERIC0, var->location); > + EXPECT_EQ(0u, var->location_frac); > + EXPECT_TRUE(var->explicit_location); > + EXPECT_FALSE(var->is_unmatched_generic_inout); > +} > + > +TEST_F(invalidate_locations, explicit_location_frac_vertex_in_generic) > +{ > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "a", > + ir_var_shader_in); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VERT_ATTRIB_GENERIC0; > + var->location_frac = 2; > + var->explicit_location = true; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(VERT_ATTRIB_GENERIC0, var->location); > + EXPECT_EQ(2u, var->location_frac); > + EXPECT_TRUE(var->explicit_location); > + EXPECT_FALSE(var->is_unmatched_generic_inout); > +} > + > +TEST_F(invalidate_locations, vertex_in_builtin) > +{ > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "gl_Vertex", > + ir_var_shader_in); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VERT_ATTRIB_POS; > + var->explicit_location = true; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(VERT_ATTRIB_POS, var->location); > + EXPECT_EQ(0u, var->location_frac); > + EXPECT_TRUE(var->explicit_location); > + EXPECT_FALSE(var->is_unmatched_generic_inout); > +} > + > +TEST_F(invalidate_locations, vertex_in_builtin_without_explicit) > +{ > + /* This test is almost identical to vertex_in_builtin. However, > + * ir_variable::explicit_location is not. > > > Did you mean to say "...is not set"? Yes. > + * link_invalidate_variable_locations has the behavior that > non-generic > + * inputs (or outputs) are not modified. > + */ > > > It seems a little weird to be testing this case, since as I understand > it, non-generic (i.e. built-in) inputs and outputs always have > explicit_location set to true. Are you really meaning to test a corner > case that we know never happens? My temptation would be to delete this > test. > > If you really think we should keep it, I think at the very least the > comment above should be updated to say something like "Although > non-generic inputs/outputs always have explicit_location set to true, > the contract of link_invalidate_variable_locations() is that it should > not modify non-generic inputs or outputs regardless of the setting of > explicit_location." Most of the tests in this patch were created based on observing the behavior of link_invalidate_variable_locations. There were a few bits of behavior, including this bit, that I found a little surprising. It seems like more often than not, when I encounter code that has surprising behavior, there's some other piece of code that subtly depends on that behavior. So, I set to document the quirks using tests. In the next patch I decided to change the behavior, and that, naturally, necessitated a change in the test. It's perhaps a little silly in this case, but, in general, I think it has one significant value. It unambiguously says, "It was not an accident that this behavior changed." Whether or not that means we want to keep these particular unit tests is still open. :) Thoughts? > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "gl_Vertex", > + ir_var_shader_in); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VERT_ATTRIB_POS; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(VERT_ATTRIB_POS, var->location); > + EXPECT_EQ(0u, var->location_frac); > + EXPECT_FALSE(var->explicit_location); > + EXPECT_FALSE(var->is_unmatched_generic_inout); > +} > + > +TEST_F(invalidate_locations, simple_vertex_out_generic) > +{ > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "a", > + ir_var_shader_out); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VARYING_SLOT_VAR0; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(-1, var->location); > + EXPECT_EQ(0u, var->location_frac); > + EXPECT_FALSE(var->explicit_location); > + EXPECT_TRUE(var->is_unmatched_generic_inout); > +} > + > +TEST_F(invalidate_locations, vertex_out_builtin) > +{ > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "gl_FrontColor", > + ir_var_shader_out); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VARYING_SLOT_COL0; > + var->explicit_location = true; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(VARYING_SLOT_COL0, var->location); > + EXPECT_EQ(0u, var->location_frac); > + EXPECT_TRUE(var->explicit_location); > + EXPECT_FALSE(var->is_unmatched_generic_inout); > +} > + > +TEST_F(invalidate_locations, vertex_out_builtin_without_explicit) > +{ > + /* This test is almost identical to vertex_out_builtin. However, > + * ir_variable::explicit_location is not. > + * link_invalidate_variable_locations has the behavior that > non-generic > + * inputs (or outputs) are not modified. > + */ > > > I have similar feelings about this test. > > > + ir_variable *const var = > + new(mem_ctx) ir_variable(glsl_type::vec(4), > + "gl_FrontColor", > + ir_var_shader_out); > + > + EXPECT_FALSE(var->explicit_location); > + EXPECT_EQ(-1, var->location); > + > + var->location = VARYING_SLOT_COL0; > + > + ir.push_tail(var); > + > + link_invalidate_variable_locations(&ir, > + VERT_ATTRIB_GENERIC0, > + VARYING_SLOT_VAR0); > + > + EXPECT_EQ(VARYING_SLOT_COL0, var->location); > + EXPECT_EQ(0u, var->location_frac); > + EXPECT_FALSE(var->explicit_location); > + EXPECT_FALSE(var->is_unmatched_generic_inout); > +} > -- > 1.8.1.4 > > > With the *_builtin_without_explicit issues resolved, this patch is: > > Reviewed-by: Paul Berry <stereotype...@gmail.com > <mailto:stereotype...@gmail.com>> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
