<div dir="ltr">On 18 October 2013 12:17, Paul Berry <span dir="ltr"><<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="im">On 11 October 2013 11:18, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br>
</div><div class="gmail_extra"><div class="gmail_quote"><div class="im">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">From: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>><br>
<br>
Ever since the addition of interface blocks with instance names, we have<br>
had an implicit invariant:<br>
<br>
var->type->is_interface() == (var->type == var->interface_type)<br>
<br>
A similar invariant exists if var->type->fields.array->is_interface().<br></blockquote><div><br></div></div><div>Can we please spell this out in more detail? As written, it seems to be implying that the following two invariants both hold simultaneously:<br>
<br></div><div class="im"><div>var->type->is_interface() == (var->type == var->interface_type)<br></div></div><div>AND<br>var->type->fields.array->is_interface() == ...<br><br></div><div>Which is impossible because you can't access "fields.array" of an interface type.<br>
<br></div><div>I think the invariant you mean is something like this:<br><br></div><div>If var->type->is_interface(), then var->interface_type == var->type.<br>If var->type->is_array() and var->type->fields.array->is_interface(), then var->interface_type == var->type->fields.array.<br>
<br></div><div>With that fixed, this patch is:<br><br>Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>><br></div></div></div></div></blockquote><div><br>
</div><div>One other thing: would you mind adding "general-ir-test" to src/glsl/tests/.gitignore? Thanks.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><div class="h5"><br> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
However, the ir_variable constructor doesn't maintain either invariant.<br>
That seems kind of silly... and I tripped over it while writing some<br>
other code. This patch makes the constructor do the right thing, and it<br>
introduces some tests to verify that behavior.<br>
<br>
Signed-off-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>><br>
---<br>
src/glsl/Makefile.am | 16 +++++++<br>
src/glsl/ast_to_hir.cpp | 1 -<br>
src/glsl/builtin_variables.cpp | 1 -<br>
src/glsl/ir.cpp | 11 ++++-<br>
src/glsl/tests/general_ir_test.cpp | 89 ++++++++++++++++++++++++++++++++++++++<br>
5 files changed, 114 insertions(+), 4 deletions(-)<br>
create mode 100644 src/glsl/tests/general_ir_test.cpp<br>
<br>
diff --git a/src/glsl/Makefile.am b/src/glsl/Makefile.am<br>
index cbf253c..f43f49d 100644<br>
--- a/src/glsl/Makefile.am<br>
+++ b/src/glsl/Makefile.am<br>
@@ -32,6 +32,7 @@ AM_CXXFLAGS = $(VISIBILITY_CXXFLAGS)<br>
include Makefile.sources<br>
<br>
TESTS = glcpp/tests/glcpp-test \<br>
+ tests/general-ir-test \<br>
tests/optimization-test \<br>
tests/ralloc-test \<br>
tests/sampler-types-test \<br>
@@ -45,12 +46,27 @@ noinst_LTLIBRARIES = <a href="http://libglsl.la" target="_blank">libglsl.la</a> <a href="http://libglcpp.la" target="_blank">libglcpp.la</a><br>
check_PROGRAMS = \<br>
glcpp/glcpp \<br>
glsl_test \<br>
+ tests/general-ir-test \<br>
tests/ralloc-test \<br>
tests/sampler-types-test \<br>
tests/uniform-initializer-test<br>
<br>
noinst_PROGRAMS = glsl_compiler<br>
<br>
+tests_general_ir_test_SOURCES = \<br>
+ $(top_srcdir)/src/mesa/main/hash_table.c \<br>
+ $(top_srcdir)/src/mesa/main/imports.c \<br>
+ $(top_srcdir)/src/mesa/program/prog_hash_table.c\<br>
+ $(top_srcdir)/src/mesa/program/symbol_table.c \<br>
+ $(GLSL_SRCDIR)/standalone_scaffolding.cpp \<br>
+ tests/general_ir_test.cpp<br>
+tests_general_ir_test_CFLAGS = \<br>
+ $(PTHREAD_CFLAGS)<br>
+tests_general_ir_test_LDADD = \<br>
+ $(top_builddir)/src/gtest/<a href="http://libgtest.la" target="_blank">libgtest.la</a> \<br>
+ $(top_builddir)/src/glsl/<a href="http://libglsl.la" target="_blank">libglsl.la</a> \<br>
+ $(PTHREAD_LIBS)<br>
+<br>
tests_uniform_initializer_test_SOURCES = \<br>
$(top_srcdir)/src/mesa/main/hash_table.c \<br>
$(top_srcdir)/src/mesa/main/imports.c \<br>
diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp<br>
index dfa32d9..b644b22 100644<br>
--- a/src/glsl/ast_to_hir.cpp<br>
+++ b/src/glsl/ast_to_hir.cpp<br>
@@ -4876,7 +4876,6 @@ ast_interface_block::hir(exec_list *instructions,<br>
var_mode);<br>
}<br>
<br>
- var->init_interface_type(block_type);<br>
if (state->target == geometry_shader && var_mode == ir_var_shader_in)<br>
handle_geometry_shader_input_decl(state, loc, var);<br>
<br>
diff --git a/src/glsl/builtin_variables.cpp b/src/glsl/builtin_variables.cpp<br>
index ae0a03f..9d78b2e 100644<br>
--- a/src/glsl/builtin_variables.cpp<br>
+++ b/src/glsl/builtin_variables.cpp<br>
@@ -858,7 +858,6 @@ builtin_variable_generator::generate_varyings()<br>
this->per_vertex_in.construct_interface_instance();<br>
ir_variable *var = add_variable("gl_in", array(per_vertex_in_type, 0),<br>
ir_var_shader_in, -1);<br>
- var->init_interface_type(per_vertex_in_type);<br>
}<br>
if (state->target == vertex_shader || state->target == geometry_shader) {<br>
const glsl_type *per_vertex_out_type =<br>
diff --git a/src/glsl/ir.cpp b/src/glsl/ir.cpp<br>
index de9613e..54a8e40 100644<br>
--- a/src/glsl/ir.cpp<br>
+++ b/src/glsl/ir.cpp<br>
@@ -1603,8 +1603,15 @@ ir_variable::ir_variable(const struct glsl_type *type, const char *name,<br>
this->depth_layout = ir_depth_layout_none;<br>
this->used = false;<br>
<br>
- if (type && type->base_type == GLSL_TYPE_SAMPLER)<br>
- this->read_only = true;<br>
+ if (type != NULL) {<br>
+ if (type->base_type == GLSL_TYPE_SAMPLER)<br>
+ this->read_only = true;<br>
+<br>
+ if (type->is_interface())<br>
+ this->init_interface_type(type);<br>
+ else if (type->is_array() && type->fields.array->is_interface())<br>
+ this->init_interface_type(type->fields.array);<br>
+ }<br>
}<br>
<br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
diff --git a/src/glsl/tests/general_ir_test.cpp b/src/glsl/tests/general_ir_test.cpp<br>
new file mode 100644<br>
index 0000000..862fa19<br>
--- /dev/null<br>
+++ b/src/glsl/tests/general_ir_test.cpp<br>
@@ -0,0 +1,89 @@<br>
+/*<br>
+ * Copyright © 2013 Intel Corporation<br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER<br>
+ * DEALINGS IN THE SOFTWARE.<br>
+ */<br>
+#include <gtest/gtest.h><br>
+#include "main/compiler.h"<br>
+#include "main/mtypes.h"<br>
+#include "main/macros.h"<br>
+#include "ralloc.h"<br>
+#include "ir.h"<br>
+<br>
+TEST(ir_variable_constructor, interface)<br>
+{<br>
+ void *mem_ctx = ralloc_context(NULL);<br>
+<br>
+ static const glsl_struct_field f[] = {<br>
+ {<br>
+ glsl_type::vec(4),<br>
+ "v",<br>
+ false<br>
+ }<br>
+ };<br>
+<br>
+ const glsl_type *const interface =<br>
+ glsl_type::get_interface_instance(f,<br>
+ ARRAY_SIZE(f),<br>
+ GLSL_INTERFACE_PACKING_STD140,<br>
+ "simple_interface");<br>
+<br>
+ static const char name[] = "named_instance";<br>
+<br>
+ ir_variable *const v =<br>
+ new(mem_ctx) ir_variable(interface, name, ir_var_uniform);<br>
+<br>
+ EXPECT_STREQ(name, v->name);<br>
+ EXPECT_NE(name, v->name);<br>
+ EXPECT_EQ(interface, v->type);<br>
+ EXPECT_EQ(interface, v->get_interface_type());<br>
+}<br>
+<br>
+TEST(ir_variable_constructor, interface_array)<br>
+{<br>
+ void *mem_ctx = ralloc_context(NULL);<br>
+<br>
+ static const glsl_struct_field f[] = {<br>
+ {<br>
+ glsl_type::vec(4),<br>
+ "v",<br>
+ false<br>
+ }<br>
+ };<br>
+<br>
+ const glsl_type *const interface =<br>
+ glsl_type::get_interface_instance(f,<br>
+ ARRAY_SIZE(f),<br>
+ GLSL_INTERFACE_PACKING_STD140,<br>
+ "simple_interface");<br>
+<br>
+ const glsl_type *const interface_array =<br>
+ glsl_type::get_array_instance(interface, 2);<br>
+<br>
+ static const char name[] = "array_instance";<br>
+<br>
+ ir_variable *const v =<br>
+ new(mem_ctx) ir_variable(interface_array, name, ir_var_uniform);<br>
+<br>
+ EXPECT_STREQ(name, v->name);<br>
+ EXPECT_NE(name, v->name);<br>
+ EXPECT_EQ(interface_array, v->type);<br>
+ EXPECT_EQ(interface, v->get_interface_type());<br>
+}<br>
<span><font color="#888888">--<br>
1.8.1.4<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>