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

[Anuj Phogat]

On Thu, Jun 19, 2014 at 6:11 AM, Tapani Pälli  wrote:
> 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
>
Tried verifying es3conform fixes. Patch doesn't compile anymore with
mesa master.

> Signed-off-by: Tapani Pälli 
> ---
>  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,
> +d

Re: [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 
> ---
>  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 

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

[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 
---
 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..de23