On 11 October 2013 11:18, Ian Romanick <i...@freedesktop.org> wrote:
> From: Ian Romanick <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.
>
> 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>
> ---
> 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 <stereotype...@gmail.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
