Re: [Qemu-devel] [PATCH] target-ppc: fix fcmp{o,u} instructions

[Nathan Froyd]

On Sun, Dec 14, 2008 at 11:35:54AM +0100, Aurelien Jarno wrote:
> > -uint32_t helper_fcmpu (uint64_t arg1, uint64_t arg2)
> > +void helper_fcmpu (uint64_t arg1, uint64_t arg2, uint32_t crfD)
> >  {
> >      CPU_DoubleU farg1, farg2;
> >      uint32_t ret = 0;
> >      farg1.ll = arg1;
> >      farg2.ll = arg2;
> >  
> > -    if (unlikely(float64_is_signaling_nan(farg1.d) ||
> > -                 float64_is_signaling_nan(farg2.d))) {
> > -        /* sNaN comparison */
> > -        fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > +    if (unlikely(float64_is_nan(farg1.d) ||
> > +                 float64_is_nan(farg2.d))) {
> > +        ret = 0x01UL;
> > +    } else if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> > +        ret = 0x08UL;
> > +    } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> > +        ret = 0x04UL;
> >      } else {
> > -        if (float64_lt(farg1.d, farg2.d, &env->fp_status)) {
> > -            ret = 0x08UL;
> > -        } else if (!float64_le(farg1.d, farg2.d, &env->fp_status)) {
> > -            ret = 0x04UL;
> > -        } else {
> > -            ret = 0x02UL;
> > -        }
> > +        ret = 0x02UL;
> >      }
> > +
> >      env->fpscr &= ~(0x0F << FPSCR_FPRF);
> >      env->fpscr |= ret << FPSCR_FPRF;
> > -    return ret;
> > +    env->crf[crfD] = ret;
> > +    if (unlikely(ret == 0x01UL
> 
> Why do you need to test if the result is 0x01UL?
> 
> > +                 && (float64_is_signaling_nan(farg1.d) ||
> > +                     float64_is_signaling_nan(farg2.d)))) {
> 
> If at least one of the arguments is a sNaN, the result should already by
> 0x01.

My reasoning was that testing for NaNness would be relatively expensive
and that NaN input should be infrequent.  Testing for 0x01UL would
therefore be a quick check to see if we need to do more expensive
checks.  I'm happy to redo the patch in the interest of clarity.

> > +    if (unlikely (ret == 0x01UL)) {
> >          if (float64_is_signaling_nan(farg1.d) ||
> >              float64_is_signaling_nan(farg2.d)) {
> >              /* sNaN comparison */
> 
> Same here.

Ditto.

-Nathan