On 11 October 2013 11:18, Ian Romanick <i...@freedesktop.org> wrote:
> From: Ian Romanick <ian.d.roman...@intel.com>
>
> Ever since the addition of interface blocks with instance names, we have
> had an implicit invariant:
>
> var->type->is_interface() == (var->type == var->interface_type)
>
> A similar invariant exists if var->type->fields.array->is_interface().
>
Can we please spell this out in more detail? As written, it seems to be
implying that the following two invariants both hold simultaneously:
var->type->is_interface() == (var->type == var->interface_type)
AND
var->type->fields.array->is_interface() == ...
Which is impossible because you can't access "fields.array" of an interface
type.
I think the invariant you mean is something like this:
If var->type->is_interface(), then var->interface_type == var->type.
If var->type->is_array() and var->type->fields.array->is_interface(), then
var->interface_type == var->type->fields.array.
With that fixed, this patch is:
Reviewed-by: Paul Berry <stereotype...@gmail.com>
>
>
> However, the ir_variable constructor doesn't maintain either invariant.
> That seems kind of silly... and I tripped over it while writing some
> other code. This patch makes the constructor do the right thing, and it
> introduces some tests to verify that behavior.
>
> Signed-off-by: Ian Romanick <ian.d.roman...@intel.com>
> ---
> src/glsl/Makefile.am | 16 +++++++
> src/glsl/ast_to_hir.cpp | 1 -
> src/glsl/builtin_variables.cpp | 1 -
> src/glsl/ir.cpp | 11 ++++-
> src/glsl/tests/general_ir_test.cpp | 89
> ++++++++++++++++++++++++++++++++++++++
> 5 files changed, 114 insertions(+), 4 deletions(-)
> create mode 100644 src/glsl/tests/general_ir_test.cpp
>
> diff --git a/src/glsl/Makefile.am b/src/glsl/Makefile.am
> index cbf253c..f43f49d 100644
> --- a/src/glsl/Makefile.am
> +++ b/src/glsl/Makefile.am
> @@ -32,6 +32,7 @@ AM_CXXFLAGS = $(VISIBILITY_CXXFLAGS)
> include Makefile.sources
>
> TESTS = glcpp/tests/glcpp-test \
> + tests/general-ir-test \
> tests/optimization-test \
> tests/ralloc-test \
> tests/sampler-types-test \
> @@ -45,12 +46,27 @@ noinst_LTLIBRARIES = libglsl.la libglcpp.la
> check_PROGRAMS = \
> glcpp/glcpp \
> glsl_test \
> + tests/general-ir-test \
> tests/ralloc-test \
> tests/sampler-types-test \
> tests/uniform-initializer-test
>
> noinst_PROGRAMS = glsl_compiler
>
> +tests_general_ir_test_SOURCES = \
> + $(top_srcdir)/src/mesa/main/hash_table.c \
> + $(top_srcdir)/src/mesa/main/imports.c \
> + $(top_srcdir)/src/mesa/program/prog_hash_table.c\
> + $(top_srcdir)/src/mesa/program/symbol_table.c \
> + $(GLSL_SRCDIR)/standalone_scaffolding.cpp \
> + tests/general_ir_test.cpp
> +tests_general_ir_test_CFLAGS = \
> + $(PTHREAD_CFLAGS)
> +tests_general_ir_test_LDADD = \
> + $(top_builddir)/src/gtest/libgtest.la \
> + $(top_builddir)/src/glsl/libglsl.la \
> + $(PTHREAD_LIBS)
> +
> tests_uniform_initializer_test_SOURCES = \
> $(top_srcdir)/src/mesa/main/hash_table.c \
> $(top_srcdir)/src/mesa/main/imports.c \
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index dfa32d9..b644b22 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -4876,7 +4876,6 @@ ast_interface_block::hir(exec_list *instructions,
> var_mode);
> }
>
> - var->init_interface_type(block_type);
> if (state->target == geometry_shader && var_mode ==
> ir_var_shader_in)
> handle_geometry_shader_input_decl(state, loc, var);
>
> diff --git a/src/glsl/builtin_variables.cpp
> b/src/glsl/builtin_variables.cpp
> index ae0a03f..9d78b2e 100644
> --- a/src/glsl/builtin_variables.cpp
> +++ b/src/glsl/builtin_variables.cpp
> @@ -858,7 +858,6 @@ builtin_variable_generator::generate_varyings()
> this->per_vertex_in.construct_interface_instance();
> ir_variable *var = add_variable("gl_in", array(per_vertex_in_type,
> 0),
> ir_var_shader_in, -1);
> - var->init_interface_type(per_vertex_in_type);
> }
> if (state->target == vertex_shader || state->target ==
> geometry_shader) {
> const glsl_type *per_vertex_out_type =
> diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp
> index de9613e..54a8e40 100644
> --- a/src/glsl/ir.cpp
> +++ b/src/glsl/ir.cpp
> @@ -1603,8 +1603,15 @@ ir_variable::ir_variable(const struct glsl_type
> *type, const char *name,
> this->depth_layout = ir_depth_layout_none;
> this->used = false;
>
> - if (type && type->base_type == GLSL_TYPE_SAMPLER)
> - this->read_only = true;
> + if (type != NULL) {
> + if (type->base_type == GLSL_TYPE_SAMPLER)
> + this->read_only = true;
> +
> + if (type->is_interface())
> + this->init_interface_type(type);
> + else if (type->is_array() && type->fields.array->is_interface())
> + this->init_interface_type(type->fields.array);
> + }
> }
>
>
> diff --git a/src/glsl/tests/general_ir_test.cpp
> b/src/glsl/tests/general_ir_test.cpp
> new file mode 100644
> index 0000000..862fa19
> --- /dev/null
> +++ b/src/glsl/tests/general_ir_test.cpp
> @@ -0,0 +1,89 @@
> +/*
> + * 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"
> +
> +TEST(ir_variable_constructor, interface)
> +{
> + void *mem_ctx = ralloc_context(NULL);
> +
> + static const glsl_struct_field f[] = {
> + {
> + glsl_type::vec(4),
> + "v",
> + false
> + }
> + };
> +
> + const glsl_type *const interface =
> + glsl_type::get_interface_instance(f,
> + ARRAY_SIZE(f),
> + GLSL_INTERFACE_PACKING_STD140,
> + "simple_interface");
> +
> + static const char name[] = "named_instance";
> +
> + ir_variable *const v =
> + new(mem_ctx) ir_variable(interface, name, ir_var_uniform);
> +
> + EXPECT_STREQ(name, v->name);
> + EXPECT_NE(name, v->name);
> + EXPECT_EQ(interface, v->type);
> + EXPECT_EQ(interface, v->get_interface_type());
> +}
> +
> +TEST(ir_variable_constructor, interface_array)
> +{
> + void *mem_ctx = ralloc_context(NULL);
> +
> + static const glsl_struct_field f[] = {
> + {
> + glsl_type::vec(4),
> + "v",
> + false
> + }
> + };
> +
> + const glsl_type *const interface =
> + glsl_type::get_interface_instance(f,
> + ARRAY_SIZE(f),
> + GLSL_INTERFACE_PACKING_STD140,
> + "simple_interface");
> +
> + const glsl_type *const interface_array =
> + glsl_type::get_array_instance(interface, 2);
> +
> + static const char name[] = "array_instance";
> +
> + ir_variable *const v =
> + new(mem_ctx) ir_variable(interface_array, name, ir_var_uniform);
> +
> + EXPECT_STREQ(name, v->name);
> + EXPECT_NE(name, v->name);
> + EXPECT_EQ(interface_array, v->type);
> + EXPECT_EQ(interface, v->get_interface_type());
> +}
> --
> 1.8.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
