Mesa (master): i965/fs: Fix comparisions with uint negation.

[Eric Anholt]

Module: Mesa
Branch: master
Commit: 73b0a28ba8b3e2ab917d4c729f34ddbde52c9e88
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=73b0a28ba8b3e2ab917d4c729f34ddbde52c9e88

Author: Eric Anholt <eric at anholt.net>
Date:   Mon Oct  3 15:12:10 2011 -0700

i965/fs: Fix comparisions with uint negation.

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-*

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

---

 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 7386106..c0d93c0 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 f344330..f5af9ea 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.h
+++ b/src/mesa/drivers/dri/i965/brw_fs.h
@@ -535,6 +535,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 88a9999..0c14baf 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_emit.cpp
@@ -635,6 +635,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 e523ae7..215cdcd 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;
+}