[Mesa-dev] [PATCH 03/11] i965: Fix negate with unsigned integers

[Eduardo Lima Mitev]

From: Iago Toral Quiroga <itoral at igalia.com>

For code such as:

uint tmp1 = uint(in0);
uint tmp2 = -tmp1;
float out0 = float(tmp2);

We produce code like:
mov(8)    g5<1>.xF    -g9<4,4,1>.xUD

which does not produce correct results. This code produces the
results we would expect if tmp1 and tmp2 were signed integers
instead.

It seems that a similar problem was detected and addressed when
using negations with unsigned integers as part of condionals, but
it looks like the problem has a wider impact than that.

This patch fixes the problem by preventing copy-propagation of
negated UD registers in all scenarios, not only in conditionals.

Fixes the following 8 dEQP tests:

dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uint_vertex
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uint_fragment
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec2_vertex
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec2_fragment
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec3_vertex
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec3_fragment
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec4_vertex
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec4_fragment

Note:
For some reason the mediump and lowp versions of these tests still fail but
I am not sure about the reason for that since the code we generate now seems
correct (in fact, is the same as for the highp versions). These tests would
need further investigation.
---
 src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp   | 9 ++++++---
 src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 9 ++++-----
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
index 70f417f..5dd7255 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_copy_propagation.cpp
@@ -302,9 +302,12 @@ fs_visitor::try_copy_propagate(fs_inst *inst, int arg, acp_entry *entry)
        (entry->dst.reg_offset + entry->regs_written) * 32)
       return false;
 
-   /* See resolve_ud_negate() and comment in brw_fs_emit.cpp. */
-   if (inst->conditional_mod &&
-       inst->src[arg].type == BRW_REGISTER_TYPE_UD &&
+   /* we can't generally copy-propagate UD negations because we
+    * can end up accessing the resulting values as signed integers
+    * instead. See also resolve_ud_negate() and comment in
+    * fs_generator::generate_code.
+    */
+   if (inst->src[arg].type == BRW_REGISTER_TYPE_UD &&
        entry->src.negate)
       return false;
 
diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
index 9e47dd9..562ecb7 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
+++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
@@ -318,12 +318,11 @@ try_copy_propagate(struct brw_context *brw, vec4_instruction *inst,
    if (inst->is_send_from_grf())
       return false;
 
-   /* We can't copy-propagate a UD negation into a condmod
-    * instruction, because the condmod ends up looking at the 33-bit
-    * signed accumulator value instead of the 32-bit value we wanted
+   /* we can't generally copy-propagate UD negations becuse we
+    * end up accessing the resulting values as signed integers
+    * instead. See also resolve_ud_negate().
     */
-   if (inst->conditional_mod &&
-       value.negate &&
+   if (value.negate &&
        value.type == BRW_REGISTER_TYPE_UD)
       return false;
 
-- 
2.1.3