This test looks great. For the series:
Reviewed-by: Chris Forbes <[email protected]> On Sun, Feb 2, 2014 at 5:10 AM, Pohjolainen, Topi <[email protected]> wrote: > On Sat, Feb 01, 2014 at 07:40:26AM -0800, Paul Berry wrote: >> On 1 February 2014 01:24, Pohjolainen, Topi <[email protected]> >> wrote: >> >> On Fri, Jan 31, 2014 at 01:12:09PM -0800, Paul Berry wrote: >> > --- >> > .../glsl-fs-continue-inside-do-while.shader_test | 51 >> +++++++++++++++++++++ >> > .../glsl-vs-continue-inside-do-while.shader_test | 52 >> ++++++++++++++++++++++ >> > 2 files changed, 103 insertions(+) >> > create mode 100644 >> tests/shaders/glsl-fs-continue-inside-do-while.shader_test >> > create mode 100644 >> tests/shaders/glsl-vs-continue-inside-do-while.shader_test >> > >> > diff --git >> a/tests/shaders/glsl-fs-continue-inside-do-while.shader_test >> b/tests/shaders/glsl-fs-continue-inside-do-while.shader_test >> > new file mode 100644 >> > index 0000000..9f5e491 >> > --- /dev/null >> > +++ b/tests/shaders/glsl-fs-continue-inside-do-while.shader_test >> > @@ -0,0 +1,51 @@ >> > +# From the GLSL 4.40 spec, section 6.4 (Jumps): >> > +# >> > +# The continue jump is used only in loops. It skips the remainder >> > +# of the body of the inner most loop of which it is inside. For >> > +# while and do-while loops, this jump is to the next evaluation >> of >> > +# the loop condition-expression from which the loop continues as >> > +# previously defined. >> > +# >> > +# As of 1/31/2014 (commit db8b6fb), Mesa handles "continue" inside a >> > +# do-while loop incorrectly; instead of jumping to the loop >> > +# condition-expression, it jumps to the top of the loop. This is >> > +# particularly problematic because it can lead to infinite loops. >> > +# >> > +# This test verifies correct behaviour of "continue" inside do-while >> > +# loops without risking an infinite loop. >> > + >> > +[require] >> > +GLSL >= 1.10 >> > + >> > +[vertex shader] >> > +void main() >> > +{ >> > + gl_Position = gl_Vertex; >> > +} >> > + >> > +[fragment shader] >> > +void main() >> > +{ >> > + int x = 0; >> > + int y = 0; >> > + do { // 1st iteration 2nd iteration 3rd iteration >> > + ++x; // x <- 1 x <- 2 x <- 3 >> > + if (x >= 4) // false false false >> > + break; >> >> This guarding is for the infinite looping case of the broken mesa to >> terminate >> after finite amount of iterations, right? Without this extra check the >> following >> continue would always jump to the top of the loop (as you explained) >> causing the >> final guarding "while (x < 3)" to be infinitely skipped. >> If this is the case, would it make sense to add a small comment for the >> future >> when the problem in mesa not present anymore and somebody might wonder >> why the >> extra guarding is needed? >> >> I already have a comment hinting at this ("This test verifies correct >> behaviour of 'continue' inside do-while loops without risking an infinite >> loop"). I'll expand the comment so that it's clearer. > > True, I take that comment back, I think it's fine as is. > >> >> >> Still need to add that this is one the best documented tests that I've >> seen. >> Thanks Paul, makes really nice reading! >> >> Thanks! Can I consider that a "Reviewed-by"? > > Yes. > >> >> >> > + if (x >= 2) // false true true >> > + continue; >> > + ++y; // y=1 skipped skipped >> > + } while (x < 3); // true true false >> > + >> > + // The "continue" should skip ++y on all iterations but the first, >> > + // so y should now be 1. The "continue" should not skip (x < 3) >> > + // ever, so the loop should terminate when x == 3 (not 4). >> > + if (x == 3 && y == 1) >> > + gl_FragColor = vec4(0.0, 1.0, 0.0, 1.0); >> > + else >> > + gl_FragColor = vec4(1.0, 0.0, 0.0, 1.0); >> > +} >> > + >> > +[test] >> > +draw rect -1 -1 2 2 >> > +probe all rgba 0.0 1.0 0.0 1.0 >> > diff --git >> a/tests/shaders/glsl-vs-continue-inside-do-while.shader_test >> b/tests/shaders/glsl-vs-continue-inside-do-while.shader_test >> > new file mode 100644 >> > index 0000000..aa6d3e6 >> > --- /dev/null >> > +++ b/tests/shaders/glsl-vs-continue-inside-do-while.shader_test >> > @@ -0,0 +1,52 @@ >> > +# From the GLSL 4.40 spec, section 6.4 (Jumps): >> > +# >> > +# The continue jump is used only in loops. It skips the remainder >> > +# of the body of the inner most loop of which it is inside. For >> > +# while and do-while loops, this jump is to the next evaluation >> of >> > +# the loop condition-expression from which the loop continues as >> > +# previously defined. >> > +# >> > +# As of 1/31/2014 (commit db8b6fb), Mesa handles "continue" inside a >> > +# do-while loop incorrectly; instead of jumping to the loop >> > +# condition-expression, it jumps to the top of the loop. This is >> > +# particularly problematic because it can lead to infinite loops. >> > +# >> > +# This test verifies correct behaviour of "continue" inside do-while >> > +# loops without risking an infinite loop. >> > + >> > +[require] >> > +GLSL >= 1.10 >> > + >> > +[vertex shader] >> > +void main() >> > +{ >> > + gl_Position = gl_Vertex; >> > + int x = 0; >> > + int y = 0; >> > + do { // 1st iteration 2nd iteration 3rd iteration >> > + ++x; // x <- 1 x <- 2 x <- 3 >> > + if (x >= 4) // false false false >> > + break; >> > + if (x >= 2) // false true true >> > + continue; >> > + ++y; // y=1 skipped skipped >> > + } while (x < 3); // true true false >> > + >> > + // The "continue" should skip ++y on all iterations but the first, >> > + // so y should now be 1. The "continue" should not skip (x < 3) >> > + // ever, so the loop should terminate when x == 3 (not 4). >> > + if (x == 3 && y == 1) >> > + gl_FrontColor = vec4(0.0, 1.0, 0.0, 1.0); >> > + else >> > + gl_FrontColor = vec4(1.0, 0.0, 0.0, 1.0); >> > +} >> > + >> > +[fragment shader] >> > +void main() >> > +{ >> > + gl_FragColor = gl_Color; >> > +} >> > + >> > +[test] >> > +draw rect -1 -1 2 2 >> > +probe all rgba 0.0 1.0 0.0 1.0 >> > -- >> > 1.8.5.3 >> > >> > _______________________________________________ >> > Piglit mailing list >> > [email protected] >> > http://lists.freedesktop.org/mailman/listinfo/piglit > _______________________________________________ > Piglit mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/piglit _______________________________________________ Piglit mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/piglit
