Hi;
Any comments on this approach? I have also a branch that implements a
'switch specific dead code elimination pass' but it is only enough to
fix non-conditional breaks (fs-exec-after-break.shader_test). If I
understand correctly fixing conditional breaks would need adding switch
breaks as part of IR or wrapping switch as a loop like in the patch here.
Thanks;
// Tapani
On 08/06/2014 02:21 PM, Tapani Pälli wrote:
> Patch removes old variable based logic for handling a break inside
> switch. Switch is put inside a loop so that existing infrastructure
> for loop flow control can be used for the switch, now also dead code
> elimination works properly.
>
> Possible 'continue' call inside a switch needs now special handling
> which is taken care of by detecting continue, breaking out and calling
> continue for the outside loop.
>
> Fixes following Piglit tests:
>
> fs-exec-after-break.shader_test
> fs-conditional-break.shader_test
>
> No Piglit or es3conform regressions.
>
> Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
> ---
> src/glsl/ast_to_hir.cpp | 101
> +++++++++++++++++++++++++++---------------
> src/glsl/glsl_parser_extras.h | 4 +-
> 2 files changed, 68 insertions(+), 37 deletions(-)
>
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 30b02d0..4e3c48c 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -4366,7 +4366,7 @@ ast_jump_statement::hir(exec_list *instructions,
> * loop.
> */
> if (state->loop_nesting_ast != NULL &&
> - mode == ast_continue) {
> + mode == ast_continue &&
> !state->switch_state.is_switch_innermost) {
> if (state->loop_nesting_ast->rest_expression) {
> state->loop_nesting_ast->rest_expression->hir(instructions,
> state);
> @@ -4378,19 +4378,27 @@ ast_jump_statement::hir(exec_list *instructions,
> }
>
> if (state->switch_state.is_switch_innermost &&
> + mode == ast_continue) {
> + /* Set 'continue_inside' to true. */
> + ir_rvalue *const true_val = new (ctx) ir_constant(true);
> + ir_dereference_variable *deref_continue_inside_var =
> + new(ctx)
> ir_dereference_variable(state->switch_state.continue_inside);
> + instructions->push_tail(new(ctx)
> ir_assignment(deref_continue_inside_var,
> + true_val));
> +
> + /* Break out from the switch, continue for the loop will
> + * be called right after switch. */
> + ir_loop_jump *const jump =
> + new(ctx) ir_loop_jump(ir_loop_jump::jump_break);
> + instructions->push_tail(jump);
> +
> + } else if (state->switch_state.is_switch_innermost &&
> mode == ast_break) {
> - /* Force break out of switch by setting is_break switch state.
> - */
> - ir_variable *const is_break_var =
> state->switch_state.is_break_var;
> - ir_dereference_variable *const deref_is_break_var =
> - new(ctx) ir_dereference_variable(is_break_var);
> - ir_constant *const true_val = new(ctx) ir_constant(true);
> - ir_assignment *const set_break_var =
> - new(ctx) ir_assignment(deref_is_break_var, true_val);
> -
> - instructions->push_tail(set_break_var);
> - }
> - else {
> + /* Force break out of switch by inserting a break. */
> + ir_loop_jump *const jump =
> + new(ctx) ir_loop_jump(ir_loop_jump::jump_break);
> + instructions->push_tail(jump);
> + } else {
> ir_loop_jump *const jump =
> new(ctx) ir_loop_jump((mode == ast_break)
> ? ir_loop_jump::jump_break
> @@ -4502,19 +4510,19 @@ ast_switch_statement::hir(exec_list *instructions,
> instructions->push_tail(new(ctx) ir_assignment(deref_is_fallthru_var,
> is_fallthru_val));
>
> - /* Initalize is_break state to false.
> + /* Initialize continue_inside state to false.
> */
> - ir_rvalue *const is_break_val = new (ctx) ir_constant(false);
> - state->switch_state.is_break_var =
> + state->switch_state.continue_inside =
> new(ctx) ir_variable(glsl_type::bool_type,
> - "switch_is_break_tmp",
> + "continue_inside_tmp",
> ir_var_temporary);
> - instructions->push_tail(state->switch_state.is_break_var);
> + instructions->push_tail(state->switch_state.continue_inside);
>
> - ir_dereference_variable *deref_is_break_var =
> - new(ctx) ir_dereference_variable(state->switch_state.is_break_var);
> - instructions->push_tail(new(ctx) ir_assignment(deref_is_break_var,
> - is_break_val));
> + ir_rvalue *const false_val = new (ctx) ir_constant(false);
> + ir_dereference_variable *deref_continue_inside_var =
> + new(ctx) ir_dereference_variable(state->switch_state.continue_inside);
> + instructions->push_tail(new(ctx) ir_assignment(deref_continue_inside_var,
> + false_val));
>
> state->switch_state.run_default =
> new(ctx) ir_variable(glsl_type::bool_type,
> @@ -4522,13 +4530,46 @@ ast_switch_statement::hir(exec_list *instructions,
> ir_var_temporary);
> instructions->push_tail(state->switch_state.run_default);
>
> + /* Loop around the switch is used for flow control. */
> + ir_loop * loop = new(ctx) ir_loop();
> + instructions->push_tail(loop);
> +
> /* Cache test expression.
> */
> - test_to_hir(instructions, state);
> + test_to_hir(&loop->body_instructions, state);
>
> /* Emit code for body of switch stmt.
> */
> - body->hir(instructions, state);
> + body->hir(&loop->body_instructions, state);
> +
> + /* Insert a break at the end to exit loop. */
> + ir_loop_jump *jump = new(ctx) ir_loop_jump(ir_loop_jump::jump_break);
> + loop->body_instructions.push_tail(jump);
> +
> + /* If we are inside loop, check if continue got called inside switch. */
> + if (state->loop_nesting_ast != NULL) {
> + ir_rvalue *const true_val = new (ctx) ir_constant(true);
> + ir_dereference_variable *deref_continue_inside =
> + new(ctx)
> ir_dereference_variable(state->switch_state.continue_inside);
> + ir_expression *cond = new(ctx) ir_expression(ir_binop_all_equal,
> + true_val,
> + deref_continue_inside);
> + ir_if *irif = new(ctx) ir_if(cond);
> + ir_loop_jump *jump = new(ctx)
> ir_loop_jump(ir_loop_jump::jump_continue);
> +
> + if (state->loop_nesting_ast != NULL) {
> + if (state->loop_nesting_ast->rest_expression) {
> +
> state->loop_nesting_ast->rest_expression->hir(&irif->then_instructions,
> + state);
> + }
> + if (state->loop_nesting_ast->mode ==
> + ast_iteration_statement::ast_do_while) {
> +
> state->loop_nesting_ast->condition_to_hir(&irif->then_instructions, state);
> + }
> + }
> + irif->then_instructions.push_tail(jump);
> + instructions->push_tail(irif);
> + }
>
> hash_table_dtor(state->switch_state.labels_ht);
>
> @@ -4652,18 +4693,6 @@ ast_case_statement::hir(exec_list *instructions,
> {
> labels->hir(instructions, state);
>
> - /* Conditionally set fallthru state based on break state. */
> - ir_constant *const false_val = new(state) ir_constant(false);
> - ir_dereference_variable *const deref_is_fallthru_var =
> - new(state)
> ir_dereference_variable(state->switch_state.is_fallthru_var);
> - ir_dereference_variable *const deref_is_break_var =
> - new(state) ir_dereference_variable(state->switch_state.is_break_var);
> - ir_assignment *const reset_fallthru_on_break =
> - new(state) ir_assignment(deref_is_fallthru_var,
> - false_val,
> - deref_is_break_var);
> - instructions->push_tail(reset_fallthru_on_break);
> -
> /* Guard case statements depending on fallthru state. */
> ir_dereference_variable *const deref_fallthru_guard =
> new(state)
> ir_dereference_variable(state->switch_state.is_fallthru_var);
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index ce66e2f..7a90b37 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -40,9 +40,11 @@ struct glsl_switch_state {
> /** Temporary variables needed for switch statement. */
> ir_variable *test_var;
> ir_variable *is_fallthru_var;
> - ir_variable *is_break_var;
> class ast_switch_statement *switch_nesting_ast;
>
> + /** Used to detect if 'continue' was called inside a switch. */
> + ir_variable *continue_inside;
> +
> /** Used to set condition if 'default' label should be chosen. */
> ir_variable *run_default;
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
