[Mesa-dev] [PATCH 2/3] i965/fs: Fix comparisions with uint negation.

[Ian Romanick]

On 10/03/2011 03:41 PM, Eric Anholt wrote:
> The condmod instruction ends up generating garbage condition codes,
> because apparently the comparison happens on the accumulator value (33
> bits for UD), not the truncated value that would be written.
>
> Fixes fs-op-neg-*
> ---
>   src/mesa/drivers/dri/i965/brw_fs.cpp         |   13 +++++++++++++
>   src/mesa/drivers/dri/i965/brw_fs.h           |    1 +
>   src/mesa/drivers/dri/i965/brw_fs_emit.cpp    |   10 ++++++++++
>   src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |   25 +++++++++++++++++++++++++
>   4 files changed, 49 insertions(+), 0 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 2000180..555d26d 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -1347,6 +1347,19 @@ fs_visitor::register_coalesce()
>   	    interfered = true;
>   	    break;
>   	 }
> +
> +	 /* The accumulator result appears to get used for the
> +	  * conditional modifier generation.  When negating a UD
> +	  * value, there is a 33rd bit generated for the sign in the
> +	  * accumulator value, so now you can't check, for example,
> +	  * equality with a 32-bit value.  See piglit fs-op-neg-uint.
> +	  */
> +	 if (scan_inst->conditional_mod&&
> +	     inst->src[0].negate&&
> +	     inst->src[0].type == BRW_REGISTER_TYPE_UD) {
> +	    interfered = true;
> +	    break;
> +	 }
>         }
>         if (interfered) {
>   	 continue;
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 56181a3..7a89413 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -534,6 +534,7 @@ public:
>   			       fs_inst *last_rhs_inst);
>      void emit_assignment_writes(fs_reg&l, fs_reg&r,
>   			       const glsl_type *type, bool predicated);
> +   void resolve_ud_negate(fs_reg *reg);
>
>      struct brw_reg interp_reg(int location, int channel);
>      int setup_uniform_values(int loc, const glsl_type *type);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> index 4c158fe..7025347 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
> @@ -636,6 +636,16 @@ fs_visitor::generate_code()
>
>         for (unsigned int i = 0; i<  3; i++) {
>   	 src[i] = brw_reg_from_fs_reg(&inst->src[i]);
> +
> +	 /* The accumulator result appears to get used for the
> +	  * conditional modifier generation.  When negating a UD
> +	  * value, there is a 33rd bit generated for the sign in the
> +	  * accumulator value, so now you can't check, for example,
> +	  * equality with a 32-bit value.  See piglit fs-op-neg-uvec4.
> +	  */
> +	 assert(!inst->conditional_mod ||
> +		inst->src[i].type != BRW_REGISTER_TYPE_UD ||
> +		!inst->src[i].negate);
>         }
>         dst = brw_reg_from_fs_reg(&inst->dst);
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index 07ea84f..24cc23c 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -317,6 +317,9 @@ fs_visitor::visit(ir_expression *ir)
>         if (intel->gen<  5)
>   	 temp.type = op[0].type;
>
> +      resolve_ud_negate(&op[0]);
> +      resolve_ud_negate(&op[1]);
> +
>         inst = emit(BRW_OPCODE_CMP, temp, op[0], op[1]);
>         inst->conditional_mod = brw_conditional_for_comparison(ir->operation);
>         emit(BRW_OPCODE_AND, this->result, this->result, fs_reg(0x1));
> @@ -377,6 +380,8 @@ fs_visitor::visit(ir_expression *ir)
>         if (intel->gen<  5)
>   	 temp.type = op[0].type;
>
> +      resolve_ud_negate(&op[0]);
> +
>         inst = emit(BRW_OPCODE_CMP, temp, op[0], fs_reg(0.0f));
>         inst->conditional_mod = BRW_CONDITIONAL_NZ;
>         inst = emit(BRW_OPCODE_AND, this->result, this->result, fs_reg(1));
> @@ -401,6 +406,9 @@ fs_visitor::visit(ir_expression *ir)
>         break;
>
>      case ir_binop_min:
> +      resolve_ud_negate(&op[0]);
> +      resolve_ud_negate(&op[1]);
> +
>         if (intel->gen>= 6) {
>   	 inst = emit(BRW_OPCODE_SEL, this->result, op[0], op[1]);
>   	 inst->conditional_mod = BRW_CONDITIONAL_L;
> @@ -416,6 +424,9 @@ fs_visitor::visit(ir_expression *ir)
>         }
>         break;
>      case ir_binop_max:
> +      resolve_ud_negate(&op[0]);
> +      resolve_ud_negate(&op[1]);
> +
>         if (intel->gen>= 6) {
>   	 inst = emit(BRW_OPCODE_SEL, this->result, op[0], op[1]);
>   	 inst->conditional_mod = BRW_CONDITIONAL_GE;
> @@ -1369,6 +1380,8 @@ fs_visitor::emit_bool_to_cond_code(ir_rvalue *ir)
>
>   	 expr->operands[i]->accept(this);
>   	 op[i] = this->result;
> +
> +	 resolve_ud_negate(&op[i]);
>         }
>
>         switch (expr->operation) {
> @@ -1984,3 +1997,15 @@ fs_visitor::emit_fb_writes()
>
>      this->current_annotation = NULL;
>   }
> +
> +void
> +fs_visitor::resolve_ud_negate(fs_reg *reg)
> +{
> +   if (reg->type != BRW_REGISTER_TYPE_UD ||
> +       !reg->negate)
> +      return;
> +
> +   fs_reg temp = fs_reg(this, glsl_type::uint_type);
> +   emit(BRW_OPCODE_MOV, temp, *reg);
> +   *reg = temp;
> +}

I suspect I know the answer, but are we sure that a later optimization 
pass won't copy propagate this extra move away?