[Mesa-dev] [PATCH] glsl/ast: Stop processing a switch-statement after an error in the init-expression

[Alejandro Piñeiro]

On 30/09/17 04:16, Ian Romanick wrote:
> From: Ian Romanick <ian.d.romanick at intel.com>
>
> This happens to work now because ir_binop_all_equal is used.  This
> causes vector typed init-expressions to produce scalar Boolean values
> after comparison.
>
> The next commit changes ir_binop_all_equal to ir_binop_equal.  Vector
> typed init-expressions will then produce vector Boolean values, and, in
> debug builds, the ir_assignment constructor will fail an assertion.
>
> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
> ---

Thanks for the detailed explanation.
Reviewed-by: Alejandro Piñeiro <apinheiro at igalia.com>

> There were a number of flaws in the original series that went undetected
> until recently.  One flaw, noted below, is fatal.  I had been testing
> the series using our CI and shader-db.  The problems went undetected for
> several reasons.  On the shader-db runs, I was using release builds that
> omitted assertions.  At least one of the failures would have tri
>
> The first failure was in changing ir_binop_all_equal to ir_binop_equal.
> The code previously relied on ir_binop_all_equal accepting mixed scalar
> and vector operands so that cases like
>
>     switch (ivec2(a, b)) {
>     case 0:
>         break;
>     }
>
> would continue to compile.  An error would be emitted in the switch
> init-statement, but the compiler would carry on trying to find more
> errors.  That problem is addressed in this patch.  Once the error in the
> switch init-statement is detected, halt compilation.
>
> The second flaw was that if-to-conditional-assign did not respect the
> write-mask of the assignment.  This would result in things like
>
>     if (cond)
>         foo.wz = bar;
>
> being transformed to 'foo.wz = csel(cond.xx, bar, foo)'.  This was solved by
> converting the write mask to a swizzle on the LHS when used in the
> conditional-select.
>
> The third flaw was that several of the patches that replace conditions
> with conditional-select "forgot" that conditional-select is
> per-component, so the vector size of the condition has to match the
> vector size of the operands.
>
> This last flaw led to the discovery of the fatal problem: ir_triop_csel
> only works on scalar and vector operands.  Statements like:
>
>     struct S s1;
>     struct S s2;
>
>     ...
>
>     if (cond)
>         s1 = s2;
>
> cannot be translated directly to a conditional-select.  Arrays and
> matrices have the same problem.  There are some possible solutions, but
> I'm not going to have time to tackle any of them right now.
>
> Assuming this patch meets with positive review, I intend to push a
> reduced series of the subset listed below.  Aside from the ordering
> change and the removal of 8 patches, the only change in the series is
> the addition of this patch.  None of the older patches are modified.
> This is still a nice clean up and a net removal of 56 lines of code.
> It's not the payoff I wanted, but I guess it's better than nothing.
>
> glsl: Fix coding standards issues in lower_if_to_cond_assign
> glsl: Fix coding standards issues in lower_vec_index_to_cond_assign
> glsl: Return ir_variable from compare_index_block
> glsl: Convert lower_vec_index_to_cond_assign to using ir_builder
> glsl: Fix coding standards issues in lower_variable_index_to_cond_assign
> glsl: Convert lower_variable_index_to_cond_assign to ir_builder
> glsl: Don't pass NULL to ir_assignment constructor when not necessary
> glsl/ast: Stop processing a switch-statement after an error in the init-expression
> glsl/ast: Use ir_binop_equal instead of ir_binop_all_equal
> glsl/ast: Convert ast_case_label::hir to ir_builder
> glsl/ast: Explicitly track the set of case labels that occur after default
> glsl/ast: Generate a more compact expression to disable execution of default case
> glsl/ast: Use logical-or instead of conditional assignment to set fallthru_var
> glsl: Move 'foo = foo;' optimization to opt_dead_code_local
> glsl: Remove spurious assertions
>
> This, along with the next series I intend to send out, is available at:
>
> https://cgit.freedesktop.org/~idr/mesa/log/?h=glsl-parser-diet
>
>  src/compiler/glsl/ast_to_hir.cpp | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> index c464549..a569318 100644
> --- a/src/compiler/glsl/ast_to_hir.cpp
> +++ b/src/compiler/glsl/ast_to_hir.cpp
> @@ -6418,6 +6418,7 @@ ast_switch_statement::hir(exec_list *instructions,
>                         state,
>                         "switch-statement expression must be scalar "
>                         "integer");
> +      return NULL;
>     }
>  
>     /* Track the switch-statement nesting in a stack-like manner.