[Mesa-stable] [Mesa-dev] [PATCH 2/2] glsl: Fix continue statements in do-while loops.

[Kenneth Graunke]

On 01/31/2014 01:12 PM, Paul Berry wrote:
> 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.
> 
> Previously, we incorrectly treated a "continue" statement as jumping
> to the top of a do-while loop.
> 
> This patch fixes the problem by replicating the loop condition when
> converting the "continue" statement to IR.  (We already do a similar
> thing in "for" loops, to ensure that "continue" causes the loop
> expression to be executed).
> 
> Fixes piglit tests:
> - glsl-fs-continue-inside-do-while.shader_test
> - glsl-vs-continue-inside-do-while.shader_test
> - glsl-fs-continue-in-switch-in-do-while.shader_test
> - glsl-vs-continue-in-switch-in-do-while.shader_test
> 
> Cc: mesa-stable at lists.freedesktop.org
> ---
>  src/glsl/ast_to_hir.cpp | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 950f513..8d096ad 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -4029,17 +4029,22 @@ ast_jump_statement::hir(exec_list *instructions,
>  	 _mesa_glsl_error(& loc, state,
>  			  "break may only appear in a loop or a switch");
>        } else {
> -	 /* For a loop, inline the for loop expression again,
> -	  * since we don't know where near the end of
> -	  * the loop body the normal copy of it
> -	  * is going to be placed.
> +	 /* For a loop, inline the for loop expression again, since we don't
> +	  * know where near the end of the loop body the normal copy of it is
> +	  * going to be placed.  Same goes for the condition for a do-while
> +	  * loop.
>  	  */
>  	 if (state->loop_nesting_ast != NULL &&
> -	     mode == ast_continue &&
> -	     state->loop_nesting_ast->rest_expression) {
> -	    state->loop_nesting_ast->rest_expression->hir(instructions,
> -							  state);
> -	 }
> +	     mode == ast_continue) {
> +            if (state->loop_nesting_ast->rest_expression) {
> +               state->loop_nesting_ast->rest_expression->hir(instructions,
> +                                                             state);
> +            }
> +            if (state->loop_nesting_ast->mode ==
> +                ast_iteration_statement::ast_do_while) {
> +               state->loop_nesting_ast->condition_to_hir(instructions, state);
> +            }
> +         }
>  
>  	 if (state->switch_state.is_switch_innermost &&
>  	     mode == ast_break) {
> 

Hmm.  This seems different than any of the approaches we talked about on
#dri-devel, but I think it should work out fine.  The expression could
have function with side effects, which will get executed again...but
that's precisely what you want in this case.

Longer term we really ought to clean up all this loop code, but...that's
been apparent for a while.

Thanks for the fix, Paul!
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20140201/4a1bee90/attachment.pgp>