[Mesa-dev] [PATCH v2] glsl: handle a switch where default is in the middle of cases

[Roland Scheidegger]

Am 19.06.2014 15:11, schrieb Tapani Pälli:
> This fixes following tests in es3conform:
> 
>    shaders.switch.default_not_last_dynamic_vertex
>    shaders.switch.default_not_last_dynamic_fragment
> 
> and makes following tests in Piglit pass:
> 
>    glsl-1.30/execution/switch/fs-default-notlast-fallthrough
>    glsl-1.30/execution/switch/fs-default_notlast
> 
> No Piglit regressions.
> 
> v2: take away unnecessary ir_if, just use conditional assignment
> 
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
> ---
>  src/glsl/ast_to_hir.cpp       | 84 +++++++++++++++++++++++++++++++++++++++++--
>  src/glsl/glsl_parser_extras.h |  3 ++
>  2 files changed, 84 insertions(+), 3 deletions(-)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 132a955..2a105c6 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -4503,6 +4503,12 @@ ast_switch_statement::hir(exec_list *instructions,
>     instructions->push_tail(new(ctx) ir_assignment(deref_is_break_var,
>                                                    is_break_val));
>  
> +   state->switch_state.run_default =
> +      new(ctx) ir_variable(glsl_type::bool_type,
> +                             "run_default_tmp",
> +                             ir_var_temporary);
> +   instructions->push_tail(state->switch_state.run_default);
> +
>     /* Cache test expression.
>      */
>     test_to_hir(instructions, state);
> @@ -4557,8 +4563,72 @@ ir_rvalue *
>  ast_case_statement_list::hir(exec_list *instructions,
>                               struct _mesa_glsl_parse_state *state)
>  {
> -   foreach_list_typed (ast_case_statement, case_stmt, link, & this->cases)
> -      case_stmt->hir(instructions, state);
> +   exec_list default_case, after_default, tmp;
> +
> +   foreach_list_typed (ast_case_statement, case_stmt, link, & this->cases) {
> +      case_stmt->hir(&tmp, state);
> +
> +      /* Default case. */
> +      if (state->switch_state.previous_default && default_case.is_empty()) {
> +         default_case.append_list(&tmp);
> +         continue;
> +      }
> +
> +      /* If default case found, append 'after_default' list. */
> +      if (!default_case.is_empty())
> +         after_default.append_list(&tmp);
> +      else
> +         instructions->append_list(&tmp);
> +   }
> +
> +   /* Handle the default case. This is done here because default might not be
> +    * the last case. We need to add checks against following cases first to see
> +    * if default should be chosen or not.
> +    */
> +   if (!default_case.is_empty()) {
> +
> +      /* Default case was the last one, no checks required. */
> +      if (after_default.is_empty()) {
> +         instructions->append_list(&default_case);
> +         return NULL;
> +      }
> +
> +      ir_rvalue *const true_val = new (state) ir_constant(true);
> +      ir_dereference_variable *deref_run_default_var =
> +         new(state) ir_dereference_variable(state->switch_state.run_default);
> +
> +      /* Choose to run default case initially, following conditional
> +       * assignments might change this.
> +       */
> +      ir_assignment *const init_var =
> +         new(state) ir_assignment(deref_run_default_var, true_val);
> +      instructions->push_tail(init_var);
> +
> +      foreach_list(n, &after_default) {
> +         ir_instruction *ir = (ir_instruction *) n;
> +         ir_assignment *assign = ir->as_assignment();
> +
> +         if (!assign)
> +            continue;
> +
> +         /* Clone the check between case label and init expression. */
> +         ir_expression *exp = (ir_expression*) assign->condition;
> +         ir_expression *clone = exp->clone(state, NULL);
> +
> +         ir_dereference_variable *deref_var =
> +            new(state) ir_dereference_variable(state->switch_state.run_default);
> +         ir_rvalue *const false_val = new (state) ir_constant(false);
> +
> +         ir_assignment *const set_false =
> +            new(state) ir_assignment(deref_var, false_val, clone);
> +
> +         instructions->push_tail(set_false);
> +      }
> +
> +      /* Append default case and all cases after it. */
> +      instructions->append_list(&default_case);
> +      instructions->append_list(&after_default);
> +   }
>  
>     /* Case statements do not have r-values. */
>     return NULL;
> @@ -4718,9 +4788,17 @@ ast_case_label::hir(exec_list *instructions,
>        }
>        state->switch_state.previous_default = this;
>  
> +      /* Set fallthru condition on 'run_default' bool. */
> +      ir_dereference_variable *deref_run_default =
> +         new(ctx) ir_dereference_variable(state->switch_state.run_default);
> +      ir_rvalue *const cond_true = new(ctx) ir_constant(true);
> +      ir_expression *test_cond = new(ctx) ir_expression(ir_binop_all_equal,
> +                                                        cond_true,
> +                                                        deref_run_default);
> +
>        /* Set falltrhu state. */
>        ir_assignment *set_fallthru =
> -         new(ctx) ir_assignment(deref_fallthru_var, true_val);
> +         new(ctx) ir_assignment(deref_fallthru_var, true_val, test_cond);
>  
>        instructions->push_tail(set_fallthru);
>     }
> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
> index a59090f..de23d33 100644
> --- a/src/glsl/glsl_parser_extras.h
> +++ b/src/glsl/glsl_parser_extras.h
> @@ -43,6 +43,9 @@ struct glsl_switch_state {
>     ir_variable *is_break_var;
>     class ast_switch_statement *switch_nesting_ast;
>  
> +   /** Used to set condition if 'default' label should be chosen. */
> +   ir_variable *run_default;
> +
>     /** Table of constant values already used in case labels */
>     struct hash_table *labels_ht;
>     class ast_case_label *previous_default;
> 

Nice to see switch statements finally getting fixed - one of the very
few generically useful piglit tests I did... Despite having implemented
the switch logic for tgsi for llvmpipe (too bad nothing but d3d10 can
actually emit these opcodes...) I wouldn't really be able to spot errors
here, but it looks alright to me, so

Reviewed-by: Roland Scheidegger <sroland at vmware.com>