[Piglit] [PATCH] piglit: Add tests to validate switch statements in OpenGL 3.0.
Paul Berry
stereotype441 at gmail.com
Tue Aug 23 09:25:06 PDT 2011
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:
[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).
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).
> +
> +[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.
More information about the Piglit
mailing list