Re: [Mesa-dev] [PATCH] glsl: implement switch flow control using a loop
Tapani Pälli 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
> // 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
>> ---
>> 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(ne
Re: [Mesa-dev] [PATCH] glsl: implement switch flow control using a loop
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
> ---
> 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,
> -
[Mesa-dev] [PATCH] glsl: implement switch flow control using a loop
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
---
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_st
