[Piglit] [PATCH] piglit: Add tests to validate switch statements in OpenGL 3.0.
Ian Romanick
idr at freedesktop.org
Wed Aug 24 12:00:23 PDT 2011
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/23/2011 09:25 AM, Paul Berry wrote:
> On 21 August 2011 18:27, Dan McCabe <zen3d.linux at gmail.com> wrote:
>> Two sets of tests are included in this change set.
>>
>> The first set of 26 tests validates compiler support. Included are tests
>> that test both positvely and negatively that the compiler processed valid
>> switch statements and detected syntax errors.
>>
>> The second set of 3 tests validates that the execution the the generated
>> code is correct. This set generate four quadrants of color. In the test of
>> the switch statement alone, these quadrants are red, green, blue and white.
>> The second and third tests validate interactions between loops and switch
>> statement, nesting one inside the other. The loops compute "random" colors
>> at each pixel where white was drawn in the first test.
>
> Have you checked these tests against any OpenGL implementations other
> than Mesa? If so, it would be nice to mention which implementation
> (and how well it performed) in the commit message. If not, I
> recommend doing so--it can be really helpful in catching
> misunderstandings of the spec as well as cases where the de facto
> standard implementation doesn't match the spec (we sometimes have to
> make mesa less than perfectly conformant so that it can run shaders
> developed using other OpenGL implementations, and when we have to do
> this it's nice to be able to make an informed decision).
>
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-01.frag b/tests/spec/glsl-1.30/execution/switch/switch-01.frag
>> new file mode 100644
>> index 0000000..94beb59
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-01.frag
>> @@ -0,0 +1,33 @@
>> +#version 130
>> +
>> +#define PI 3.1415927
>> +
>> +in vec4 tex_coord;
>> +
>> +void main()
>> +{
>> + // use 0.5 * (sin(PI * foo) + 1) as a computation
>> + // to map -1..1 => 0..1
>> + int x = int(0.99 * (sin(PI * tex_coord.x) + 1));
>> + int y = int(0.99 * (sin(PI * tex_coord.y) + 1));
>
> It took me a long time to work out what this expression does. It
> looks like what you are trying to achieve here is: x=1 if tex_coord.x
>> 0, 0 otherwise. Any reason not to do it this way?
>
> int x = (tex_coord.x > 0) ? 1 : 0;
> int y = (tex_coord.y > 0) ? 1 : 0;
>
> Also, consider renaming x to x_gt_zero to reflect the fact that it
> isn't actually the x coordinate of anything, just a flag indicating
> the sign of x (and similarly for y).
>
>> +
>> + int v = 2 * y + x;
>> +
>> + switch (v) {
>> + case 0:
>> + gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);
>> + break;
>> + case 1:
>> + gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>> + break;
>> + case 2:
>> + gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0);
>> + break;
>> + case 3:
>> + gl_FragColor = vec4(1.0, 1.0, 1.0, 1.0);
>> + break;
>> + default:
>> + gl_FragColor = vec4(0.0, 0.0, 0.0, 1.0);
>> + break;
>> + }
>> +}
>> \ No newline at end of file
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-01.frag.sav b/tests/spec/glsl-1.30/execution/switch/switch-01.frag.sav
>> new file mode 100644
>> index 0000000..e6763d2
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-01.frag.sav
>> @@ -0,0 +1,8 @@
>> +#version 130
>> +const float canary = 0.125;
>> +uniform sampler2DShadow tex;
>> +in vec4 tex_coord;
>> +void main() {
>> + float s = texture(tex, tex_coord.xyy);
>> + gl_FragColor = vec4(s, canary, canary, canary);
>> +}
>
> Was this file included by accident? I don't see it referred to anywhere.
>
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-01.shader_test b/tests/spec/glsl-1.30/execution/switch/switch-01.shader_test
>> new file mode 100644
>> index 0000000..1dc7e84
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-01.shader_test
>> @@ -0,0 +1,44 @@
>> +# [description]
>> +# Test switch statement with
>> +# - color mapping to (x,y)
>> +# - (-1,-1)..(0,0) => red
>> +# - (0,-1)..(1,0) => green
>> +# - (-1,0)..(0,1) => blue
>> +# - (0,0)..(1,1) => white
>> +# This pattern repeats periodically in x,y
>> +
>> +[require]
>> +GLSL >= 1.30
>> +
>> +[vertex shader file]
>> +switch-01.vert
>> +
>> +[fragment shader file]
>> +switch-01.frag
>
> Minor nit-pick: for shader_runner tests like these where there is a
> one-to-one correspondence between the shader_test file and the vertex
> and fragment shaders, I generally prefer to put the vertex and
> fragment shaders inline in the file using "[vertex shader]" and
> "[fragment shader]", so that you don't have to open 3 files to
> understand what the test is doing.
>
>> +
>> +[test]
>> +# draw from -1..1 in both x and y
>> +draw rect -1 -1 2 2
>> +
>> +# color comparison pass
>> +# probe in space 0..1 in both x and y
>> +
>> +# expect red, (case 0)
>> +# drawn in (-1,-1)..(0,0),
>> +# probed at (-0.5,-0.5) in drawing coords
>> +relative probe rgba (0.25, 0.25) (1.0, 0.0, 0.0, 1.0);
>> +
>> +# expect green, (case 1)
>> +# drawn in (0,-1)..(1,0),
>> +# probed at (0.5,-0.5) in drawing coords
>> +relative probe rgba (0.75, 0.25) (0.0, 1.0, 0.0, 1.0);
>> +
>> +# expect blue, (case 2)
>> +# drawn in (-1,0)..(0,1),
>> +# probed at (-0.5,0.5) in drawing coords
>> +relative probe rgba (0.25, 0.75) (0.0, 0.0, 1.0, 1.0);
>> +
>> +# expect white, (case 3)
>> +# drawn in (0,0)..(1,1),
>> +# probed at (0.5,0.5) in drawing coords
>> +relative probe rgba (0.75, 0.75) (1.0, 1.0, 1.0, 1.0);
>
> Consider using "ortho 0 1 0 1" at the start of your test, and
> "gl_Position = gl_ModelViewProjectionMatrix * gl_Vertex;" at the top
> of your vertex shader. That way gl_Vertex will be in the same
> (0,0)..(1,1) coordinate system used by "relative probe rgba" and you
> won't need to do mental gymnastics to figure out which part of the
> screen to probe :)
>
> Another pattern I've found useful is to write the shader so that its
> behavior depends on the setting of uniforms rather than on screen
> coordinates. It makes for a more boring-looking render, but it can be
> a lot easier to reason about what's going on in the test. For
> example, here's an alternative version of this test using uniforms:
In this case, I think it's useful to use something in a varying. We do
need to test using varyings as switch-statement controls. A lot of
tests will do some modification of the incoming gl_Vertex to map it to
[0,1] in the vertex shader.
This might be a place to test integer varyings, too. Assuming ortho is
used, a vertex shader like this should do the trick:
#version 130
flat out ivec2 control;
void main() {
gl_Position = gl_ModelViewProjectionMatrix * gl_Vertex;
control = ivec2(gl_Vertex * 2.0);
}
Then the fragment shader can just do some simple integer math on control
to generate the correct switch value for the quadrant. For that, I'd
recommend something like:
value = (control.y << 2) | control.x;
The switch statement would handle 0, 1, 4, and 5. Other values should
generate the error color.
> [require]
> GLSL >= 1.30
>
> [vertex shader]
> #version 130
>
> void main()
> {
> gl_Position = gl_Vertex;
> }
>
> [fragment shader]
> #version 130
>
> uniform int v;
>
> void main()
> {
> switch (v) {
> case 0: gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0); break;
> case 1: gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0); break;
> case 2: gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0); break;
> case 3: gl_FragColor = vec4(1.0, 1.0, 1.0, 1.0); break;
> default: gl_FragColor = vec4(0.0, 0.0, 0.0, 1.0); break;
> }
> }
>
> [test]
> uniform int v -1
> draw rect -1 -1 2 2
> probe all rgba 0 0 0 1
> uniform int v 0
> draw rect -1 -1 2 2
> probe all rgba 1 0 0 1
> uniform int v 1
> draw rect -1 -1 2 2
> probe all rgba 0 1 0 1
> uniform int v 2
> draw rect -1 -1 2 2
> probe all rgba 0 0 1 1
> uniform int v 3
> draw rect -1 -1 2 2
> probe all rgba 1 1 1 1
> uniform int v 4
> draw rect -1 -1 2 2
> probe all rgba 0 0 0 1
>
> An advantage of this testing style is that it's easily suited to
> testing a feature in both the fragment shader and the vertex shader
> (for example, if you replace gl_FragColor with gl_FrontColor, you
> could move the switch statement into the vertex shader and test it
> there). Testing features in both the vertex and fragment shaders is
> really useful for architectures that use different code paths to
> compile vertex and fragment shaders (like i965).
Yes. This is very important. All execution tests have to run both
places if possible.
> Note: I verified that the above test passes on my nVidia reference
> platform, but it generates an assertion failure on i965 IronLake with
> the latest Mesa master. We should figure out why that is.
>
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-01.vert b/tests/spec/glsl-1.30/execution/switch/switch-01.vert
>> new file mode 100644
>> index 0000000..99e983c
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-01.vert
>> @@ -0,0 +1,6 @@
>> +#version 130
>> +out vec4 tex_coord;
>> +void main() {
>> + gl_Position = gl_Vertex;
>> + tex_coord = gl_Vertex;
>> +}
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-02.frag b/tests/spec/glsl-1.30/execution/switch/switch-02.frag
>> new file mode 100644
>> index 0000000..70d3bf6
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-02.frag
>> @@ -0,0 +1,67 @@
>> +#version 130
>> +
>> +#define PI 3.1415927
>> +
>> +in vec4 tex_coord;
>> +
>> +void main()
>> +{
>> + // use 0.5 * (sin(PI * foo) + 1) as a computation
>> + // to map -1..1 => 0..1
>> + int x = int(0.99 * (sin(PI * tex_coord.x) + 1));
>> + int y = int(0.99 * (sin(PI * tex_coord.y) + 1));
>> +
>> + // generate pseudo-random numbers based on x,y
>> + int rand0 = int(32.0 * (sin(tex_coord.x + tex_coord.y) + 1));
>> + int rand1 = int(32.0 * (cos(tex_coord.x * tex_coord.y) + 1));
>> + int rand2 = int(32.0 * (cos(tex_coord.x - tex_coord.y) + 1));
>> +
>> + float r = 0.0;
>> + float g = 0.0;
>> + float b = 0.0;
>> + float dr = 0.2;
>> + float dg = 0.4;
>> + float db = 0.6;
>> +
>> + int v = 2 * y + x;
>> +
>> + switch (v) {
>> + case 0:
>> + gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);
>> + break;
>> + case 1:
>> + gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>> + break;
>> + case 2:
>> + gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0);
>> + break;
>> + case 3:
>> + // compute some random colors using a while loop
>> + while (rand0 >= 0) {
>> + r = r + dr;
>> + if (r > 1.0) {
>> + r = 0.0;
>> + }
>> + rand0 = rand0 - 1;
>> + }
>> + while (rand1 >= 0) {
>> + g = g + dg;
>> + if (g > 1.0) {
>> + g = 0.0;
>> + }
>> + rand1 = rand1 - 1;
>> + }
>> + while (rand2 >= 0) {
>> + b = b + db;
>> + if (b > 1.0) {
>> + b = 0.0;
>> + }
>> + rand2 = rand2 - 1;
>> + }
>> + gl_FragColor = vec4(r, g, b, 1.0);
>> + break;
>> + default:
>> + gl_FragColor = vec4(0.0, 0.0, 0.0, 1.0);
>> + break;
>> + }
>> +}
>> \ No newline at end of file
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-02.shader_test b/tests/spec/glsl-1.30/execution/switch/switch-02.shader_test
>> new file mode 100644
>> index 0000000..92afe2c
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-02.shader_test
>> @@ -0,0 +1,43 @@
>> +# [description]
>> +# Test switch statement with
>> +# - color mapping to (x,y)
>> +# - (-1,-1)..(0,0) => red
>> +# - (0,-1)..(1,0) => green
>> +# - (-1,0)..(0,1) => blue
>> +# - (0,0)..(1,1) => multi-colored unpredicable mess
>> +# This pattern repeats periodically in x,y
>> +# The key point of this test is that the red, green,
>> +# and blue quads are drawn as expected and the interaction
>> +# betw the while loop and the switch doesn't hang
>> +# the GPU but terminates nicely.
>
> It would be preferable to make the while loops perform an actual
> computation whose output can be checked (e.g. multiplication by
> repeated addition). Just checking that the GPU terminated is nice,
> but it's a much less thorough test (for example, this test wouldn't
> catch a bug which caused no code to be emitted when a while loop
> appears inside a switch statement).
Yes. The test is better than nothing as-is, but it could be immensely
more useful.
>> +
>> +[require]
>> +GLSL >= 1.30
>> +
>> +[vertex shader file]
>> +switch-02.vert
>> +
>> +[fragment shader file]
>> +switch-02.frag
>> +
>> +[test]
>> +# draw from -1..1 in both x and y
>> +draw rect -1 -1 2 2
>> +
>> +# color comparison pass
>> +# probe in space 0..1 in both x and y
>> +
>> +# expect red, (case 0)
>> +# drawn in (-1,-1)..(0,0),
>> +# probed at (-0.5,-0.5) in drawing coords
>> +relative probe rgba (0.25, 0.25) (1.0, 0.0, 0.0, 1.0);
>> +
>> +# expect green, (case 1)
>> +# drawn in (0,-1)..(1,0),
>> +# probed at (0.5,-0.5) in drawing coords
>> +relative probe rgba (0.75, 0.25) (0.0, 1.0, 0.0, 1.0);
>> +
>> +# expect blue, (case 2)
>> +# drawn in (-1,0)..(0,1),
>> +# probed at (-0.5,0.5) in drawing coords
>> +relative probe rgba (0.25, 0.75) (0.0, 0.0, 1.0, 1.0);
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-02.vert b/tests/spec/glsl-1.30/execution/switch/switch-02.vert
>> new file mode 100644
>> index 0000000..99e983c
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-02.vert
>> @@ -0,0 +1,6 @@
>> +#version 130
>> +out vec4 tex_coord;
>> +void main() {
>> + gl_Position = gl_Vertex;
>> + tex_coord = gl_Vertex;
>> +}
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-03.frag b/tests/spec/glsl-1.30/execution/switch/switch-03.frag
>> new file mode 100644
>> index 0000000..dc99c15
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-03.frag
>> @@ -0,0 +1,57 @@
>> +#version 130
>> +
>> +#define PI 3.1415927
>> +
>> +in vec4 tex_coord;
>> +
>> +void main()
>> +{
>> + // use 0.5 * (sin(PI * foo) + 1) as a computation
>> + // to map -1..1 => 0..1
>> + int x = int(0.99 * sin(PI * tex_coord.x) + 1);
>> + int y = int(0.99 * sin(PI * tex_coord.y) + 1);
>> +
>> + // generate a pseudo-random number based on x,y
>> + int rand = int(32.0 * (sin(tex_coord.x + tex_coord.y) + 1));
>> +
>> + float r = 0.0;
>> + float g = 0.0;
>> + float b = 0.0;
>> + float dr = 0.2;
>> + float dg = 0.4;
>> + float db = 0.6;
>> +
>> + int v = 2 * y + x;
>> +
>> + // generate some random colors with this while loop
>> + while (rand >= 0) {
>> + r = r + dr;
>> + if (r > 1.0) {
>> + r = 0.0;
>> + }
>> + g = g + dg;
>> + if (g > 1.0) {
>> + g = 0.0;
>> + }
>> + b = b + db;
>> + if (b > 1.0) {
>> + b = 0.0;
>> + }
>> + rand = rand - 1;
>> +
>> + switch (v) {
>> + case 0:
>> + gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0);
>> + break;
>> + case 1:
>> + gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0);
>> + break;
>> + case 2:
>> + gl_FragColor = vec4(0.0, 0.0, 1.0, 1.0);
>> + break;
>> + case 3:
>> + gl_FragColor = vec4(r, g, b, 1.0);
>> + break;
>> + }
>> + }
>> +}
>> \ No newline at end of file
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-03.shader_test b/tests/spec/glsl-1.30/execution/switch/switch-03.shader_test
>> new file mode 100644
>> index 0000000..bbe37d7
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-03.shader_test
>> @@ -0,0 +1,43 @@
>> +# [description]
>> +# Test switch statement with
>> +# - color mapping to (x,y)
>> +# - (-1,-1)..(0,0) => red
>> +# - (0,-1)..(1,0) => green
>> +# - (-1,0)..(0,1) => blue
>> +# - (0,0)..(1,1) => multi-colored unpredicable mess
>> +# This pattern repeats periodically in x,y
>> +# The key point of this test is that the red, green,
>> +# and blue quads are drawn as expected and the interaction
>> +# betw the while loop and the switch doesn't hang
>> +# the GPU but terminates nicely.
>
> Same comment as above.
>
>> +
>> +[require]
>> +GLSL >= 1.30
>> +
>> +[vertex shader file]
>> +switch-03.vert
>> +
>> +[fragment shader file]
>> +switch-03.frag
>> +
>> +[test]
>> +# draw from -1..1 in both x and y
>> +draw rect -1 -1 2 2
>> +
>> +# color comparison pass
>> +# probe in space 0..1 in both x and y
>> +
>> +# expect red, (case 0)
>> +# drawn in (-1,-1)..(0,0),
>> +# probed at (-0.5,-0.5) in drawing coords
>> +relative probe rgba (0.25, 0.25) (1.0, 0.0, 0.0, 1.0);
>> +
>> +# expect green, (case 1)
>> +# drawn in (0,-1)..(1,0),
>> +# probed at (0.5,-0.5) in drawing coords
>> +relative probe rgba (0.75, 0.25) (0.0, 1.0, 0.0, 1.0);
>> +
>> +# expect blue, (case 2)
>> +# drawn in (-1,0)..(0,1),
>> +# probed at (-0.5,0.5) in drawing coords
>> +relative probe rgba (0.25, 0.75) (0.0, 0.0, 1.0, 1.0);
>> diff --git a/tests/spec/glsl-1.30/execution/switch/switch-03.vert b/tests/spec/glsl-1.30/execution/switch/switch-03.vert
>> new file mode 100644
>> index 0000000..99e983c
>> --- /dev/null
>> +++ b/tests/spec/glsl-1.30/execution/switch/switch-03.vert
>> @@ -0,0 +1,6 @@
>> +#version 130
>> +out vec4 tex_coord;
>> +void main() {
>> + gl_Position = gl_Vertex;
>> + tex_coord = gl_Vertex;
>> +}
>> --
>> 1.7.4.1
>>
>
> Is this the first in a planned series of patches to test switch
> statement functionality, or do you feel this is sufficient? This
> level of testing seems very light to me, especially when it comes to
> shader_runner tests, but I don't want to be overly critical if you
> still have some more patches on the way.
>
> Here a (partial) list of some other shader_runner tests that I would
> really like to see:
>
> - switch statement containing if statement: check that the explicit if
> statement doesn't interfere with the if statements that are generated
> to implement the switch statement.
> - switch statement containing if statement that does a "break" in one
> branch: check that the "break" properly exits the switch statement,
> and that execution falls through to the next case if the "break" is
> not executed.
> - switch statement containing a return statement (inside main())
> - switch statement containing a return statement (inside a function
> other than main())
> - switch statement containing a discard statement
> - loop containing a switch statement containing a continue statement:
> check that the "continue" properly restarts the loop (it doesn't try
> to restart the switch statement by accident, for example).
> - loop containing a switch statement containing a break statement:
> check that the "break" breaks out of the switch statement but not out
> of the loop.
> - switch statement containing a loop containing a break statement:
> check that the "break" breaks out of the loop but not out of the
> switch statement.
> - switch statement where the switch expression is not a variable or a
> constant but a complex expression.
> - switch statement where the switch expression is a complex expression
> with side effects: verify that the side effects happen exactly once.
> - switch statement where the switch expression contains a
> post-increment or post-decrement: verify that the increment or
> decrement happens at the right time.
>
> And of course, once we figure out why we get an assertion failure from
> my alternative test using uniforms, we may want to add a test case for
> whatever causes that.
>
> BTW: if this is your first exposure to Piglit I totally understand if
> it's taking longer than expected to write the tests you want to write.
> Let me know if there's anything I can to do help shorten your
> learning curve--I've gotten a lot of practice cranking out Piglit
> tests over the last few months.
> _______________________________________________
> Piglit mailing list
> Piglit at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/piglit
>
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
iEYEARECAAYFAk5VSkcACgkQX1gOwKyEAw9slgCgjAiLGaDBQRuZeKn7BMZfe7qz
gKkAnjCuWzmMZdUl3tgzjp31TlDBMY0D
=FRky
-----END PGP SIGNATURE-----
More information about the Piglit
mailing list