<div dir="ltr">On 9 August 2013 11:59, Ian Romanick <span dir="ltr"><<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This would have been easier to review if it had been broken up.  It's a pretty huge patch.<br></blockquote><div><br></div><div>Yeah, sorry about that.  I have an idea for how to split it into 8 patches, so v2 should probably be a lot easier to review.<br>
 </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
In addition to the comments below, it may be worth adding a couple other tests in follow-on work.  All of the cases that determine whether or not .length() is available also apply to non-constant array indexing.  You can't use .length() on implicitly sized arrays, and you can't use non-constant indexing either.  It may be worth adding tests for those.</blockquote>
<div><br></div><div>Yeah, that's a good point.  I'll make a note of that to work on in the future.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
<br>
On 08/05/2013 08:51 AM, Paul Berry wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/tests/spec/glsl-1.50/compiler/gs-input-sizing-implied-length-blocks.geom b/tests/spec/glsl-1.50/compiler/gs-input-sizing-implied-length-blocks.geom<br>
new file mode 100644<br>
index 0000000..b3c7594<br>
--- /dev/null<br>
+++ b/tests/spec/glsl-1.50/<u></u>compiler/gs-input-sizing-<u></u>implied-length-blocks.geom<br>
@@ -0,0 +1,31 @@<br>
+// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:<br>
+//<br>
+//   All geometry shader input unsized array declarations will be<br>
+//   sized by an earlier input layout qualifier, when present, as per<br>
+//   the following table.<br>
+//<br>
+// Followed by a table mapping each allowed input layout qualifier to<br>
+// the corresponding input length.<br>
+//<br>
+// This test verifies that if an unsized array declaration follows an<br>
+// input layout qualifier, the size is implied.  This test verifies<br>
+// the case for input interface blocks.<br>
+//<br>
+// [config]<br>
+// expect_result: pass<br>
+// glsl_version: 1.50<br>
+// check_link: false<br>
+// [end config]<br>
+<br>
+#version 150<br>
+<br>
+layout(lines) in;<br>
+<br>
+in blk {<br>
+  vec4 Color;<br>
+} inst[];<br>
+<br>
+int foo()<br>
+{<br>
+  return inst.length();<br>
+}<br>
</blockquote>
<br></div></div>
This test could be a bit stronger by doing something like:<br>
<br>
uniform int foo[inst.length() == 2 ? 1 : -1];<br>
<br>
This checks not only that the length is set, but that it is 2.</blockquote><div><br></div><div>That's a good point.  I'll update this test (and others like it).<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/tests/spec/glsl-1.50/compiler/gs-input-sizing-layout-inconsistent-with-prev-length-blocks.geom b/tests/spec/glsl-1.50/compiler/gs-input-sizing-layout-inconsistent-with-prev-length-blocks.geom<br>
new file mode 100644<br>
index 0000000..4f0ec10<br>
--- /dev/null<br>
+++ b/tests/spec/glsl-1.50/compiler/gs-input-sizing-layout-inconsistent-with-prev-length-blocks.geom<br>
@@ -0,0 +1,23 @@<br>
+// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:<br>
+//<br>
+//   It is a compile-time error if a layout declaration's array size<br>
+//   (from table above) does not match any array size specified in<br>
+//   declarations of an input variable in the same shader.<br>
+//<br>
+// This test verifies the case where the input variable declaration<br>
+// precedes the layout declaration.  This test verifies the case for<br>
+// input interface blocks.<br>
+//<br>
+// [config]<br>
+// expect_result: fail<br>
+// glsl_version: 1.50<br>
+// check_link: false<br>
+// [end config]<br>
+<br>
+#version 150<br>
+<br>
+in blk {<br>
+  vec4 Color;<br>
+} inst[3];<br>
+<br>
+layout(lines) in;<br>
</blockquote>
<br></div></div>
This patch is big, so I may have missed the cousin test... is there also a check for inst[2] vs. layout(triangles)?</blockquote><div><br></div><div>No, there isn't.  It didn't seem worth testing all 5 possible input layouts (points, lines, lines_adjacency, triangles, and triangles_adjacency) when the same line of code in Mesa is going to be responsible for all 5 tests passing or failing.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/tests/spec/glsl-1.50/compiler/gs-input-sizing-length-after-layout-blocks.geom b/tests/spec/glsl-1.50/compiler/gs-input-sizing-length-after-layout-blocks.geom<br>
new file mode 100644<br>
index 0000000..1d5ac5b<br>
--- /dev/null<br>
+++ b/tests/spec/glsl-1.50/<u></u>compiler/gs-input-sizing-<u></u>length-after-layout-blocks.<u></u>geom<br>
@@ -0,0 +1,39 @@<br>
+// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec<br>
+// includes the following examples of compile-time errors:<br>
+//<br>
+//   // code sequence within one shader...<br>
+//   in vec4 Color1[];    // size unknown<br>
+//   ...Color1.length()...// illegal, length() unknown<br>
+//   in vec4 Color2[2];   // size is 2<br>
+//   ...Color1.length()...// illegal, Color1 still has no size<br>
+//   in vec4 Color3[3];   // illegal, input sizes are inconsistent<br>
+//   layout(lines) in;    // legal, input size is 2, matching<br>
+//   in vec4 Color4[3];   // illegal, contradicts layout<br>
+//   ...Color1.length()...// legal, length() is 2, Color1 sized by layout() (*)<br>
+//   layout(lines) in;    // legal, matches other layout() declaration<br>
+//   layout(triangles) in;// illegal, does not match earlier layout() declaration<br>
+//<br>
+// This test verifies the case marked with (*), namely that it is<br>
+// legal to call .length() on an unsized geometry shader input array,<br>
+// if an input layout declaration occurs between the declaration of<br>
+// the input and the call to .length().  This test verifies the case<br>
+// for input interface blocks.<br>
+//<br>
+// [config]<br>
+// expect_result: pass<br>
+// glsl_version: 1.50<br>
+// check_link: false<br>
+// [end config]<br>
+<br>
+#version 150<br>
+<br>
+in blk1 {<br>
+  vec4 Color;<br>
+} inst1[];<br>
+<br>
+layout(lines) in;<br>
+<br>
+int foo()<br>
+{<br>
+  return inst1.length();<br>
+}<br>
</blockquote>
<br></div></div>
Didn't I already comment on this test?</blockquote><div><br></div><div>There's a subtle difference.  The previous test declared the layout, then the block, then tried to check the length.  This test declares the block first then the layout.  The two tests exercise different code paths in Mesa, since in the former case, the block gets the correct size the moment it's created, but in the latter case, the size has to get fixed up when the layout is encountered.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/tests/spec/glsl-1.50/compiler/gs-input-sizing-length-before-layout-blocks.geom b/tests/spec/glsl-1.50/compiler/gs-input-sizing-length-before-layout-blocks.geom<br>
new file mode 100644<br>
index 0000000..364d9ff<br>
--- /dev/null<br>
+++ b/tests/spec/glsl-1.50/<u></u>compiler/gs-input-sizing-<u></u>length-before-layout-blocks.<u></u>geom<br>
@@ -0,0 +1,38 @@<br>
+// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec<br>
+// includes the following examples of compile-time errors:<br>
+//<br>
+//   // code sequence within one shader...<br>
+//   in vec4 Color1[];    // size unknown<br>
+//   ...Color1.length()...// illegal, length() unknown (*)<br>
+//   in vec4 Color2[2];   // size is 2<br>
+//   ...Color1.length()...// illegal, Color1 still has no size<br>
+//   in vec4 Color3[3];   // illegal, input sizes are inconsistent<br>
+//   layout(lines) in;    // legal, input size is 2, matching<br>
+//   in vec4 Color4[3];   // illegal, contradicts layout<br>
+//   ...Color1.length()...// legal, length() is 2, Color1 sized by layout()<br>
+//   layout(lines) in;    // legal, matches other layout() declaration<br>
+//   layout(triangles) in;// illegal, does not match earlier layout() declaration<br>
+//<br>
+// This test verifies the case marked with (*), namely that it is<br>
+// illegal to call .length() on an unsized geometry shader input<br>
+// array, even if an input layout declaration occurs later in the<br>
+// shader.  This test verifies the case for input interface blocks.<br>
+//<br>
+// [config]<br>
+// expect_result: fail<br>
+// glsl_version: 1.50<br>
+// check_link: false<br>
+// [end config]<br>
+<br>
+#version 150<br>
+<br>
+in blk1 {<br>
+  vec4 Color;<br>
+} inst1[];<br>
+<br>
+int foo()<br>
+{<br>
+  return inst1.length();<br>
+}<br>
+<br>
+layout(lines) in;<br>
</blockquote>
<br></div></div>
While I like using the array declaration method for the positive tests, just using .length() is fine for negative tests.</blockquote><div><br></div><div>Agreed.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/tests/spec/glsl-1.50/execution/gs-input-sizing-implied-length.shader_test b/tests/spec/glsl-1.50/execution/gs-input-sizing-implied-length.shader_test<br>
new file mode 100644<br>
index 0000000..7356679<br>
--- /dev/null<br>
+++ b/tests/spec/glsl-1.50/<u></u>execution/gs-input-sizing-<u></u>implied-length.shader_test<br>
@@ -0,0 +1,84 @@<br>
+# Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:<br>
+#<br>
+#   All geometry shader input unsized array declarations will be<br>
+#   sized by an earlier input layout qualifier, when present, as per<br>
+#   the following table.<br>
+#<br>
+# Followed by a table mapping each allowed input layout qualifier to<br>
+# the corresponding input length.<br>
+#<br>
+# This test verifies that if an unsized array declaration follows an<br>
+# input layout qualifier, the correct size is implied.<br>
</blockquote>
<br></div></div>
This test may not be necessary with the array-size method I mentioned above.</blockquote><div><br></div><div>Agreed.  I'll remove it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+[require]<br>
+GLSL >= 1.50<br>
+<br>
+[vertex shader]<br>
+#version 150<br>
+<br>
+in int value;<br>
+flat out int value_to_gs;<br>
+<br>
+void main()<br>
+{<br>
+  value_to_gs = value;<br>
+}<br>
+<br>
+[geometry shader]<br>
+#version 150<br>
+<br>
+layout(triangles) in;<br>
+layout(triangle_strip, max_vertices = 4) out;<br>
+flat in int value_to_gs[];<br>
+out vec4 color_to_fs;<br>
+<br>
+void main()<br>
+{<br>
+  vec4 color;<br>
+  if (value_to_gs.length() == 3)<br>
+    color = vec4(0.0, 1.0, 0.0, 1.0);<br>
+  else<br>
+    color = vec4(0.0, 1.0, 0.0, 1.0);<br>
</blockquote>
<br></div></div>
Did you intend this to set the same value in both cases?</blockquote><div><br></div><div>Oops, no I didn't.  Good thing I'm removing this test :)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/tests/spec/glsl-1.50/execution/gs-input-sizing-layout-consistent-with-static-usage.shader_test b/tests/spec/glsl-1.50/execution/gs-input-sizing-layout-consistent-with-static-usage.shader_test<br>
new file mode 100644<br>
index 0000000..96d1f7f<br>
--- /dev/null<br>
+++ b/tests/spec/glsl-1.50/<u></u>execution/gs-input-sizing-<u></u>layout-consistent-with-static-<u></u>usage.shader_test<br>
@@ -0,0 +1,82 @@<br>
+# Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:<br>
+#<br>
+# It is a link-time error if not all provided sizes (sized input<br>
+# arrays and layout size) match across all geometry shaders in the<br>
+# program.<br>
+#<br>
+# This test exercises the case where one compilation unit provides a<br>
+# size via a layout declaration, and another provides a size<br>
+# implicitly by accessing a member of an input array using a constant<br>
+# that is consistent with the size provided in the layout declaration.<br>
</blockquote>
<br></div></div>
This is a clever test. :)  How do other implementations fare?</blockquote><div><br></div><div>I don't recall.  I'll check my NVIDIA system and let you know.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
diff --git a/tests/spec/glsl-1.50/execution/gs-input-sizing-length-after-layout.shader_test b/tests/spec/glsl-1.50/execution/gs-input-sizing-length-after-layout.shader_test<br>
new file mode 100644<br>
index 0000000..95ca2b1<br>
--- /dev/null<br>
+++ b/tests/spec/glsl-1.50/<u></u>execution/gs-input-sizing-<u></u>length-after-layout.shader_<u></u>test<br>
@@ -0,0 +1,92 @@<br>
+# Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec<br>
+# includes the following examples of compile-time errors:<br>
+#<br>
+#   // code sequence within one shader...<br>
+#   in vec4 Color1[];    // size unknown<br>
+#   ...Color1.length()...// illegal, length() unknown<br>
+#   in vec4 Color2[2];   // size is 2<br>
+#   ...Color1.length()...// illegal, Color1 still has no size<br>
+#   in vec4 Color3[3];   // illegal, input sizes are inconsistent<br>
+#   layout(lines) in;    // legal, input size is 2, matching<br>
+#   in vec4 Color4[3];   // illegal, contradicts layout<br>
+#   ...Color1.length()...// legal, length() is 2, Color1 sized by layout() (*)<br>
+#   layout(lines) in;    // legal, matches other layout() declaration<br>
+#   layout(triangles) in;// illegal, does not match earlier layout() declaration<br>
+#<br>
+# This test verifies the case marked with (*), namely that the correct<br>
+# value is returned by .length() on an unsized geometry shader input<br>
+# array, if an input layout declaration occurs between the declaration<br>
+# of the input and the call to .length().<br>
</blockquote>
<br></div></div>
Same comments as for the previous flavor of this test.</blockquote><div><br></div><div>Yup, this test can go away too.<br><br></div><div>Thanks for your review, Ian.  Revised version (split into 8 patches for easier review) coming soon!<br>
</div></div></div></div>