[Piglit] [PATCH v2] gs: Test rules related to geometry shader input sizes.
Paul Berry
stereotype441 at gmail.com
Mon Aug 12 14:30:35 PDT 2013
On 9 August 2013 11:59, Ian Romanick <idr at freedesktop.org> wrote:
> This would have been easier to review if it had been broken up. It's a
> pretty huge patch.
>
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.
>
> 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.
Yeah, that's a good point. I'll make a note of that to work on in the
future.
>
>
> On 08/05/2013 08:51 AM, Paul Berry wrote:
>
>> 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
>> new file mode 100644
>> index 0000000..b3c7594
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.50/**compiler/gs-input-sizing-**
>> implied-length-blocks.geom
>> @@ -0,0 +1,31 @@
>> +// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:
>> +//
>> +// All geometry shader input unsized array declarations will be
>> +// sized by an earlier input layout qualifier, when present, as per
>> +// the following table.
>> +//
>> +// Followed by a table mapping each allowed input layout qualifier to
>> +// the corresponding input length.
>> +//
>> +// This test verifies that if an unsized array declaration follows an
>> +// input layout qualifier, the size is implied. This test verifies
>> +// the case for input interface blocks.
>> +//
>> +// [config]
>> +// expect_result: pass
>> +// glsl_version: 1.50
>> +// check_link: false
>> +// [end config]
>> +
>> +#version 150
>> +
>> +layout(lines) in;
>> +
>> +in blk {
>> + vec4 Color;
>> +} inst[];
>> +
>> +int foo()
>> +{
>> + return inst.length();
>> +}
>>
>
> This test could be a bit stronger by doing something like:
>
> uniform int foo[inst.length() == 2 ? 1 : -1];
>
> This checks not only that the length is set, but that it is 2.
That's a good point. I'll update this test (and others like it).
>
> 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
>> new file mode 100644
>> index 0000000..4f0ec10
>> --- /dev/null
>> +++
>> b/tests/spec/glsl-1.50/compiler/gs-input-sizing-layout-inconsistent-with-prev-length-blocks.geom
>> @@ -0,0 +1,23 @@
>> +// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:
>> +//
>> +// It is a compile-time error if a layout declaration's array size
>> +// (from table above) does not match any array size specified in
>> +// declarations of an input variable in the same shader.
>> +//
>> +// This test verifies the case where the input variable declaration
>> +// precedes the layout declaration. This test verifies the case for
>> +// input interface blocks.
>> +//
>> +// [config]
>> +// expect_result: fail
>> +// glsl_version: 1.50
>> +// check_link: false
>> +// [end config]
>> +
>> +#version 150
>> +
>> +in blk {
>> + vec4 Color;
>> +} inst[3];
>> +
>> +layout(lines) in;
>>
>
> This patch is big, so I may have missed the cousin test... is there also a
> check for inst[2] vs. layout(triangles)?
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.
>
>
> 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
>> new file mode 100644
>> index 0000000..1d5ac5b
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.50/**compiler/gs-input-sizing-**
>> length-after-layout-blocks.**geom
>> @@ -0,0 +1,39 @@
>> +// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec
>> +// includes the following examples of compile-time errors:
>> +//
>> +// // code sequence within one shader...
>> +// in vec4 Color1[]; // size unknown
>> +// ...Color1.length()...// illegal, length() unknown
>> +// in vec4 Color2[2]; // size is 2
>> +// ...Color1.length()...// illegal, Color1 still has no size
>> +// in vec4 Color3[3]; // illegal, input sizes are inconsistent
>> +// layout(lines) in; // legal, input size is 2, matching
>> +// in vec4 Color4[3]; // illegal, contradicts layout
>> +// ...Color1.length()...// legal, length() is 2, Color1 sized by
>> layout() (*)
>> +// layout(lines) in; // legal, matches other layout() declaration
>> +// layout(triangles) in;// illegal, does not match earlier layout()
>> declaration
>> +//
>> +// This test verifies the case marked with (*), namely that it is
>> +// legal to call .length() on an unsized geometry shader input array,
>> +// if an input layout declaration occurs between the declaration of
>> +// the input and the call to .length(). This test verifies the case
>> +// for input interface blocks.
>> +//
>> +// [config]
>> +// expect_result: pass
>> +// glsl_version: 1.50
>> +// check_link: false
>> +// [end config]
>> +
>> +#version 150
>> +
>> +in blk1 {
>> + vec4 Color;
>> +} inst1[];
>> +
>> +layout(lines) in;
>> +
>> +int foo()
>> +{
>> + return inst1.length();
>> +}
>>
>
> Didn't I already comment on this test?
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.
>
>
> 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
>> new file mode 100644
>> index 0000000..364d9ff
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.50/**compiler/gs-input-sizing-**
>> length-before-layout-blocks.**geom
>> @@ -0,0 +1,38 @@
>> +// Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec
>> +// includes the following examples of compile-time errors:
>> +//
>> +// // code sequence within one shader...
>> +// in vec4 Color1[]; // size unknown
>> +// ...Color1.length()...// illegal, length() unknown (*)
>> +// in vec4 Color2[2]; // size is 2
>> +// ...Color1.length()...// illegal, Color1 still has no size
>> +// in vec4 Color3[3]; // illegal, input sizes are inconsistent
>> +// layout(lines) in; // legal, input size is 2, matching
>> +// in vec4 Color4[3]; // illegal, contradicts layout
>> +// ...Color1.length()...// legal, length() is 2, Color1 sized by
>> layout()
>> +// layout(lines) in; // legal, matches other layout() declaration
>> +// layout(triangles) in;// illegal, does not match earlier layout()
>> declaration
>> +//
>> +// This test verifies the case marked with (*), namely that it is
>> +// illegal to call .length() on an unsized geometry shader input
>> +// array, even if an input layout declaration occurs later in the
>> +// shader. This test verifies the case for input interface blocks.
>> +//
>> +// [config]
>> +// expect_result: fail
>> +// glsl_version: 1.50
>> +// check_link: false
>> +// [end config]
>> +
>> +#version 150
>> +
>> +in blk1 {
>> + vec4 Color;
>> +} inst1[];
>> +
>> +int foo()
>> +{
>> + return inst1.length();
>> +}
>> +
>> +layout(lines) in;
>>
>
> While I like using the array declaration method for the positive tests,
> just using .length() is fine for negative tests.
Agreed.
>
>
> 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
>> new file mode 100644
>> index 0000000..7356679
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.50/**execution/gs-input-sizing-**
>> implied-length.shader_test
>> @@ -0,0 +1,84 @@
>> +# Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:
>> +#
>> +# All geometry shader input unsized array declarations will be
>> +# sized by an earlier input layout qualifier, when present, as per
>> +# the following table.
>> +#
>> +# Followed by a table mapping each allowed input layout qualifier to
>> +# the corresponding input length.
>> +#
>> +# This test verifies that if an unsized array declaration follows an
>> +# input layout qualifier, the correct size is implied.
>>
>
> This test may not be necessary with the array-size method I mentioned
> above.
Agreed. I'll remove it.
>
>
> +
>> +[require]
>> +GLSL >= 1.50
>> +
>> +[vertex shader]
>> +#version 150
>> +
>> +in int value;
>> +flat out int value_to_gs;
>> +
>> +void main()
>> +{
>> + value_to_gs = value;
>> +}
>> +
>> +[geometry shader]
>> +#version 150
>> +
>> +layout(triangles) in;
>> +layout(triangle_strip, max_vertices = 4) out;
>> +flat in int value_to_gs[];
>> +out vec4 color_to_fs;
>> +
>> +void main()
>> +{
>> + vec4 color;
>> + if (value_to_gs.length() == 3)
>> + color = vec4(0.0, 1.0, 0.0, 1.0);
>> + else
>> + color = vec4(0.0, 1.0, 0.0, 1.0);
>>
>
> Did you intend this to set the same value in both cases?
Oops, no I didn't. Good thing I'm removing this test :)
>
>
> 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
>> new file mode 100644
>> index 0000000..96d1f7f
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.50/**execution/gs-input-sizing-**
>> layout-consistent-with-static-**usage.shader_test
>> @@ -0,0 +1,82 @@
>> +# Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec says:
>> +#
>> +# It is a link-time error if not all provided sizes (sized input
>> +# arrays and layout size) match across all geometry shaders in the
>> +# program.
>> +#
>> +# This test exercises the case where one compilation unit provides a
>> +# size via a layout declaration, and another provides a size
>> +# implicitly by accessing a member of an input array using a constant
>> +# that is consistent with the size provided in the layout declaration.
>>
>
> This is a clever test. :) How do other implementations fare?
I don't recall. I'll check my NVIDIA system and let you know.
>
>
> 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
>> new file mode 100644
>> index 0000000..95ca2b1
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.50/**execution/gs-input-sizing-**
>> length-after-layout.shader_**test
>> @@ -0,0 +1,92 @@
>> +# Section 4.3.8.1 (Input Layout Qualifiers) of the GLSL 1.50 spec
>> +# includes the following examples of compile-time errors:
>> +#
>> +# // code sequence within one shader...
>> +# in vec4 Color1[]; // size unknown
>> +# ...Color1.length()...// illegal, length() unknown
>> +# in vec4 Color2[2]; // size is 2
>> +# ...Color1.length()...// illegal, Color1 still has no size
>> +# in vec4 Color3[3]; // illegal, input sizes are inconsistent
>> +# layout(lines) in; // legal, input size is 2, matching
>> +# in vec4 Color4[3]; // illegal, contradicts layout
>> +# ...Color1.length()...// legal, length() is 2, Color1 sized by
>> layout() (*)
>> +# layout(lines) in; // legal, matches other layout() declaration
>> +# layout(triangles) in;// illegal, does not match earlier layout()
>> declaration
>> +#
>> +# This test verifies the case marked with (*), namely that the correct
>> +# value is returned by .length() on an unsized geometry shader input
>> +# array, if an input layout declaration occurs between the declaration
>> +# of the input and the call to .length().
>>
>
> Same comments as for the previous flavor of this test.
Yup, this test can go away too.
Thanks for your review, Ian. Revised version (split into 8 patches for
easier review) coming soon!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20130812/39fc1299/attachment-0001.html>
More information about the Piglit
mailing list