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

[Anuj Phogat]

On Mon, Jan 19, 2015 at 3:32 AM, Eduardo Lima Mitev <elima at igalia.com> wrote:
> 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
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Changes look good to me. Verified that patch now generates correct
code for the example in the comment.

Reviewed-by: Anuj Phogat <anuj.phogat at gmail.com>