[Mesa-dev] [PATCH] glsl: don't flatten if-blocks with dynamic array indices

[Marek Olšák]

Thanks!

Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Marek

On Thu, Nov 17, 2016 at 10:59 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> From: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> This fixes the regression of radeonsi in
> glsl-1.10/execution/variable-indexing/vs-output-array-vec3-index-wr
> caused by commit 74e39de9324d2d2333cda6adca50ae2a3fc36de2.
> ---
>  src/compiler/glsl/lower_if_to_cond_assign.cpp | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/glsl/lower_if_to_cond_assign.cpp b/src/compiler/glsl/lower_if_to_cond_assign.cpp
> index ae048be..37f1ec8 100644
> --- a/src/compiler/glsl/lower_if_to_cond_assign.cpp
> +++ b/src/compiler/glsl/lower_if_to_cond_assign.cpp
> @@ -79,20 +79,21 @@ public:
>     ~ir_if_to_cond_assign_visitor()
>     {
>        _mesa_set_destroy(this->condition_variables, NULL);
>     }
>
>     ir_visitor_status visit_enter(ir_if *);
>     ir_visitor_status visit_leave(ir_if *);
>
>     bool found_unsupported_op;
>     bool found_expensive_op;
> +   bool found_dynamic_arrayref;
>     bool is_then;
>     bool progress;
>     gl_shader_stage stage;
>     unsigned then_cost;
>     unsigned else_cost;
>     unsigned min_branch_cost;
>     unsigned max_depth;
>     unsigned depth;
>
>     struct set *condition_variables;
> @@ -141,22 +142,27 @@ check_ir_node(ir_instruction *ir, void *data)
>            var->data.mode == ir_var_shader_out)
>           v->found_unsupported_op = true;
>        break;
>     }
>
>     /* SSBO, images, atomic counters are handled by ir_type_call */
>     case ir_type_texture:
>        v->found_expensive_op = true;
>        break;
>
> +   case ir_type_dereference_array: {
> +      ir_dereference_array *deref = ir->as_dereference_array();
> +
> +      if (deref->array_index->ir_type != ir_type_constant)
> +         v->found_dynamic_arrayref = true;
> +   } /* fall-through */
>     case ir_type_expression:
> -   case ir_type_dereference_array:
>     case ir_type_dereference_record:
>        if (v->is_then)
>           v->then_cost++;
>        else
>           v->else_cost++;
>        break;
>
>     default:
>        break;
>     }
> @@ -222,42 +228,51 @@ ir_visitor_status
>  ir_if_to_cond_assign_visitor::visit_leave(ir_if *ir)
>  {
>     bool must_lower = this->depth-- > this->max_depth;
>
>     /* Only flatten when beyond the GPU's maximum supported nesting depth. */
>     if (!must_lower && this->min_branch_cost == 0)
>        return visit_continue;
>
>     this->found_unsupported_op = false;
>     this->found_expensive_op = false;
> +   this->found_dynamic_arrayref = false;
>     this->then_cost = 0;
>     this->else_cost = 0;
>
>     ir_assignment *assign;
>
>     /* Check that both blocks don't contain anything we can't support. */
>     this->is_then = true;
>     foreach_in_list(ir_instruction, then_ir, &ir->then_instructions) {
>        visit_tree(then_ir, check_ir_node, this);
>     }
>
>     this->is_then = false;
>     foreach_in_list(ir_instruction, else_ir, &ir->else_instructions) {
>        visit_tree(else_ir, check_ir_node, this);
>     }
>
>     if (this->found_unsupported_op)
>        return visit_continue; /* can't handle inner unsupported opcodes */
>
> -   /* Skip if the branch cost is high enough or if there's an expensive op. */
> +   /* Skip if the branch cost is high enough or if there's an expensive op.
> +    *
> +    * Also skip if non-constant array indices were encountered, since those
> +    * can be out-of-bounds for a not-taken branch, and so generating an
> +    * assignment would be incorrect. In the case of must_lower, it's up to the
> +    * backend to deal with any potential fall-out (perhaps by translating the
> +    * assignments to hardware-predicated moves).
> +    */
>     if (!must_lower &&
>         (this->found_expensive_op ||
> +        this->found_dynamic_arrayref ||
>          MAX2(this->then_cost, this->else_cost) >= this->min_branch_cost))
>        return visit_continue;
>
>     void *mem_ctx = ralloc_parent(ir);
>
>     /* Store the condition to a variable.  Move all of the instructions from
>      * the then-clause of the if-statement.  Use the condition variable as a
>      * condition for all assignments.
>      */
>     ir_variable *const then_var =
> --
> 2.7.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev