Re: [Qemu-devel] [PATCH 3/3] target-ppc: fix sNaN propagation

[Aurelien Jarno <aurelien@aurel32.net>]

On Mon, Jan 10, 2011 at 06:14:18PM -0600, Peter Maydell wrote:
> On 2 January 2011 08:39, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > The current FPU code returns 0.0 if one of the operand is a
> > signaling NaN and the VXSNAN exception is disabled.
> >
> > fload_invalid_op_excp() doesn't return a qNaN in case of a VXSNAN
> > exception as the operand should be propagated instead of a new
> > qNaN to be generated. Fix that by calling fload_invalid_op_excp()
> > only for the exception generation (if enabled), and use the softfloat
> > code to correctly compute the result.
> >
> > Cc: Alexander Graf <agraf@suse.de>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> > @@ -1410,10 +1418,10 @@ uint64_t helper_frsp (uint64_t arg)
> >     if (unlikely(float64_is_signaling_nan(farg.d))) {
> >         /* sNaN square root */
> >        farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > -    } else {
> > -       f32 = float64_to_float32(farg.d, &env->fp_status);
> > -       farg.d = float32_to_float64(f32, &env->fp_status);
> >     }
> > +    f32 = float64_to_float32(farg.d, &env->fp_status);
> > +    farg.d = float32_to_float64(f32, &env->fp_status);
> > +
> >     return farg.ll;
> >  }
> 
> Most of these changes are to ignoring the result from
> fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN),
> but this one leaves it assigning a value to farg.ll -- is there
> any reason for that? (It looks like the assignment gets
> immediately overwritten by the assignment to farg.d later.)

It is actually a mistake, though the result is correct. I'll resend the
patch after fixing that, thanks for the review.

> > @@ -1460,11 +1468,11 @@ uint64_t helper_fres (uint64_t arg)
> >     if (unlikely(float64_is_signaling_nan(farg.d))) {
> >         /* sNaN reciprocal */
> >         farg.ll = fload_invalid_op_excp(POWERPC_EXCP_FP_VXSNAN);
> > -    } else {
> > -        farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> > -        f32 = float64_to_float32(farg.d, &env->fp_status);
> > -        farg.d = float32_to_float64(f32, &env->fp_status);
> >     }
> > +    farg.d = float64_div(float64_one, farg.d, &env->fp_status);
> > +    f32 = float64_to_float32(farg.d, &env->fp_status);
> > +    farg.d = float32_to_float64(f32, &env->fp_status);
> > +
> >     return farg.ll;
> >  }
> >
> 
> Ditto for this hunk.
> 
> Looks plausible to me other than that.
> 

Same here.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net