[Mesa-dev] [PATCH 5/6] glsl/tests: Unit test vertex shader in / out with link_invalidate_variable_locations

Paul Berry stereotype441 at gmail.com
Tue Oct 22 23:45:03 CEST 2013


On 22 October 2013 13:22, Ian Romanick <idr at freedesktop.org> wrote:

> On 10/18/2013 01:31 PM, Paul Berry wrote:
> > On 11 October 2013 11:18, 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 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.romanick at intel.com
> >     <mailto:ian.d.romanick at 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?
>

Not sure if you're asking for my thoughts or for others', since I thought I
already said that I'd prefer to see the test removed.  So just to be clear:

I'm a huge fan of unit testing and I'm really glad to see all the unit
testing that you're adding to the code base.  But I believe it's possible
to go too far with unit testing, and one way to go too far is to create
tests that are so unrealistic that the test could fail even if the code has
no bugs.  That's exactly what's happening here.  The fact is, it doesn't
matter what link_invalidate_variable_locations does to built-in variables
that lack an explicit location, because there's no such thing.  In fact,
that's exactly why patch 6 is correct, because it only changes the
behaviour of link_invalidate_variable_locations for an impossible case.

IMHO the right way to handle impossible cases like this one is to add tests
or assertions to make sure they can never occur.  Patches 2-4 do a great
job of that.

I'll grant you that changing the unit test in patch 6 sends an unambiguous
message that you changed the behaviour on purpose.  But I believe you could
send that message even more unambiguously by simply saying in the commit
message for patch 6 "Note that this patch changes how
link_invalidate_variable_locations handles the impossible case of built-ins
that don't have an explicit location set.  It used to do X.  Now it does
Y.  There's no user-visible change because all built-ins have an explicit
location set, as verified by the {vertex,geometry,fragment}_builtin unit
tests."  And the problem with trying to send a message by writing a unit
test is that the presence of a unit test carries another, far more
powerful, message.  It says to future maintainers: "this behaviour of the
code is important.  It's so important that I've decided to test it in
isolation.  It's so important that it's worth making a future maintainer go
to the extra work of fixing up the unit test in case they ever want to
change the behaviour."  And that message isn't true for this test.

The reason you wrote the test in the first place is that it was hard to
tell what the code was intended to do, so you wrote tests to figure out
what it did do.  That's great, and I'm glad you did it.  Writing the test
wasn't a waste of time.  But now we have even more information: now we know
that some of the things that link_invalidate_variable_locations did was
unimportant.  The right thing to document that this behaviour doesn't
matter, and remove the test so that nobody in the future has to re-do the
investigation you did.


>
> >     +   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 <stereotype441 at gmail.com
> > <mailto:stereotype441 at gmail.com>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131022/e82b6ddf/attachment-0001.html>


More information about the mesa-dev mailing list