Tapani Pälli <tapani.pa...@intel.com> writes: > 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; >
I like this solution because it has the advantage that it doesn't
increase the complexity of the IR that different back-ends will then
have to handle, as defining a new ir_switch instruction would. -- No
need for back-ends to re-implement the same logic to lower it to a chain
of if statements themselves.
Sure, it might be more optimal to implement the switch statement as a
jump table on some architectures in the rare cases where it's faster
than a chain or a binary tree of if conditionals. But a majority of the
hardware we care about won't be able to do that anyway because the
argument of the switch statement can be an arbitrary non-uniform
expression, and for the minority that can handle it I'll be surprised if
it makes any significant difference.
Aside from the minor nit-pick below, this patch is:
Reviewed-by: Francisco Jerez <curroje...@riseup.net>
> // 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);
Isn't this ir_expression redundant? AFAICT continue_inside is already a
boolean value you could use as argument for ir_if.
>> + 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;
>>
pgpXJhbWZg318.pgp
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
