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

[Iago Toral]

On Tue, 2015-01-20 at 19:06 -0800, Anuj Phogat wrote:
> 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.

Thanks for testing and reviewing Anuj.

I re-tested it too and realized this also fixes the mediump and lowp
versions if the tests so I'll update the commit log accordingly.

Iago

> > ---
> >  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>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev