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

[Dylan Baker]

Are you sure this is correct? Marek mentions in the referenced commit message
that these pass on softpipe, and they work on i965 and i915 just fine.

Dylan

Quoting Nicolai Hähnle (2016-11-17 13:59:26)
> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 455 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161117/789da919/attachment.sig>