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

[Nicolai Hähnle]

On 18.11.2016 02:35, Dylan Baker wrote:
> 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.

GLSL states very clearly that out-of-bounds array accesses lead to 
undefined behavior. Without this patch, we effectively turn:

   if (cond)
     array[i] = x;

into

   tmp = array[i];
   tmp = cond ? x : tmp;
   array[i] = tmp;

This is the code that radeonsi hands off to LLVM, and LLVM takes that 
code literally. This implies the assumption that i is within array 
bounds. Combined with the optimizations that LLVM does (in particular 
instruction scheduling), an out-of-bounds i will lead to an unrelated 
register being overwritten by the original value of tmp when cond if 
false. For example, array is stored in hardware registers v0 .. v3 and i 
== 4. Due to instruction scheduling, v4 corresponds to a different value 
when it is read than when it is written.

softpipe interprets the resulting TGSI literally and without instruction 
scheduling, so even if it ignores the ArrayID annotations, it will end 
up _not_ clobbering other values. I suspect i9?5 are similar.

Cheers,
Nicolai

> 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