[Mesa-dev] [PATCH] state_tracker: Fix assertion failures in conditional block movs.

[Jose Fonseca]

On 13/12/14 02:24, Eric Anholt wrote:
> If you had a conditional assignment of an array or struct (say, from the
> if-lowering pass), we'd try doing swizzle_for_size() on the aggregate
> type, and it would assertion fail due to vector_elements==0.  Instead,
> extend emit_block_mov() to handle emitting the conditional operations,
> which also means we'll have appropriate writemasks/swizzles on the CMPs
> within a struct containing various-sized members.
>
> Fixes 20 testcases in es3conform on vc4.
> ---
>   src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 57 ++++++++++++++----------------
>   1 file changed, 26 insertions(+), 31 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> index 80dd102..6bf8e9f 100644
> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
> @@ -457,7 +457,8 @@ public:
>      void renumber_registers(void);
>
>      void emit_block_mov(ir_assignment *ir, const struct glsl_type *type,
> -                       st_dst_reg *l, st_src_reg *r);
> +                       st_dst_reg *l, st_src_reg *r,
> +                       st_src_reg *cond, bool cond_swap);
>
>      void *mem_ctx;
>   };
> @@ -2390,18 +2391,20 @@ glsl_to_tgsi_visitor::process_move_condition(ir_rvalue *ir)
>
>   void
>   glsl_to_tgsi_visitor::emit_block_mov(ir_assignment *ir, const struct glsl_type *type,
> -                                     st_dst_reg *l, st_src_reg *r)
> +                                     st_dst_reg *l, st_src_reg *r,
> +                                     st_src_reg *cond, bool cond_swap)
>   {
>      if (type->base_type == GLSL_TYPE_STRUCT) {
>         for (unsigned int i = 0; i < type->length; i++) {
> -         emit_block_mov(ir, type->fields.structure[i].type, l, r);
> +         emit_block_mov(ir, type->fields.structure[i].type, l, r,
> +                        cond, cond_swap);
>         }
>         return;
>      }
>
>      if (type->is_array()) {
>         for (unsigned int i = 0; i < type->length; i++) {
> -         emit_block_mov(ir, type->fields.array, l, r);
> +         emit_block_mov(ir, type->fields.array, l, r, cond, cond_swap);
>         }
>         return;
>      }
> @@ -2413,7 +2416,7 @@ glsl_to_tgsi_visitor::emit_block_mov(ir_assignment *ir, const struct glsl_type *
>                                            type->vector_elements, 1);
>
>         for (int i = 0; i < type->matrix_columns; i++) {
> -         emit_block_mov(ir, vec_type, l, r);
> +         emit_block_mov(ir, vec_type, l, r, cond, cond_swap);
>         }
>         return;
>      }
> @@ -2421,7 +2424,22 @@ glsl_to_tgsi_visitor::emit_block_mov(ir_assignment *ir, const struct glsl_type *
>      assert(type->is_scalar() || type->is_vector());
>
>      r->type = type->base_type;
> -   emit(ir, TGSI_OPCODE_MOV, *l, *r);
> +   if (cond) {
> +      st_src_reg l_src = st_src_reg(*l);
> +      l_src.swizzle = swizzle_for_size(type->vector_elements);
> +
> +      if (native_integers) {
> +         emit(ir, TGSI_OPCODE_UCMP, *l, *cond,
> +              cond_swap ? l_src : *r,
> +              cond_swap ? *r : l_src);
> +      } else {
> +         emit(ir, TGSI_OPCODE_CMP, *l, *cond,
> +              cond_swap ? l_src : *r,
> +              cond_swap ? *r : l_src);
> +      }
> +   } else {
> +      emit(ir, TGSI_OPCODE_MOV, *l, *r);
> +   }
>      l->index++;
>      r->index++;
>   }
> @@ -2431,7 +2449,6 @@ glsl_to_tgsi_visitor::visit(ir_assignment *ir)
>   {
>      st_dst_reg l;
>      st_src_reg r;
> -   int i;
>
>      ir->rhs->accept(this);
>      r = this->result;
> @@ -2488,29 +2505,7 @@ glsl_to_tgsi_visitor::visit(ir_assignment *ir)
>         const bool switch_order = this->process_move_condition(ir->condition);
>         st_src_reg condition = this->result;
>
> -      for (i = 0; i < type_size(ir->lhs->type); i++) {
> -         st_src_reg l_src = st_src_reg(l);
> -         st_src_reg condition_temp = condition;
> -         st_src_reg op1, op2;
> -         l_src.swizzle = swizzle_for_size(ir->lhs->type->vector_elements);
> -
> -         op1 = r;
> -         op2 = l_src;
> -         if (switch_order) {
> -            op1 = l_src;
> -            op2 = r;
> -         }
> -
> -         if (native_integers) {
> -            emit(ir, TGSI_OPCODE_UCMP, l, condition_temp, op1, op2);
> -         }
> -         else {
> -            emit(ir, TGSI_OPCODE_CMP, l, condition_temp, op1, op2);
> -         }
> -
> -         l.index++;
> -         r.index++;
> -      }
> +      emit_block_mov(ir, ir->lhs->type, &l, &r, &condition, switch_order);
>      } else if (ir->rhs->as_expression() &&
>                 this->instructions.get_tail() &&
>                 ir->rhs == ((glsl_to_tgsi_instruction *)this->instructions.get_tail())->ir &&
> @@ -2527,7 +2522,7 @@ glsl_to_tgsi_visitor::visit(ir_assignment *ir)
>         new_inst->saturate = inst->saturate;
>         inst->dead_mask = inst->dst.writemask;
>      } else {
> -      emit_block_mov(ir, ir->rhs->type, &l, &r);
> +      emit_block_mov(ir, ir->rhs->type, &l, &r, NULL, false);
>      }
>   }
>
>

Looks good to me.

Reviewed-by: Jose Fonseca <jfonseca at vmware.com>