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

Paul Berry stereotype441 at gmail.com
Fri Oct 18 22:31:11 CEST 2013


On 11 October 2013 11:18, Ian Romanick <idr at freedesktop.org> wrote:

> From: Ian Romanick <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.


>
> 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>
> ---
>  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"?


> +    * 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."


> +   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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131018/23b0379f/attachment-0001.html>


More information about the mesa-dev mailing list