Re: [Qemu-devel] [PATCH 04/14] target-mips: use softfloat constants when possible

[Aurelien Jarno]

On Wed, Oct 10, 2012 at 01:09:49PM -0700, Richard Henderson wrote:
> On 10/09/2012 01:27 PM, Aurelien Jarno wrote:
> > softfloat already has a few constants defined, use them instead of
> > redefining them in target-mips.
> > 
> > Rename FLOAT_SNAN32 and FLOAT_SNAN64 to FP_TO_INT32_OVERFLOW and
> > FP_TO_INT64_OVERFLOW as even if they have the same value, they are
> > technically different (and defined differently in the MIPS ISA).
> > 
> > Remove the unused constants.
> > 
> > Signed-off-by: Aurelien Jarno <address@hidden>
> 
> Reviewed-by: Richard Henderson <address@hidden>
> 
> > @@ -2495,8 +2491,9 @@ uint64_t helper_float_cvtl_d(CPUMIPSState *env, 
> > uint64_t fdt0)
> >      set_float_exception_flags(0, &env->active_fpu.fp_status);
> >      dt2 = float64_to_int64(fdt0, &env->active_fpu.fp_status);
> >      update_fcr31(env);
> > -    if (GET_FP_CAUSE(env->active_fpu.fcr31) & (FP_OVERFLOW | FP_INVALID))
> > -        dt2 = FLOAT_SNAN64;
> > +    if (GET_FP_CAUSE(env->active_fpu.fcr31) & (FP_OVERFLOW | FP_INVALID)) {
> > +        dt2 = FP_TO_INT64_OVERFLOW;
> > +    }
> >      return dt2;
> 
> That said, the existing code you're patching is incorrect.
> 
> This code will fold to OVERFLOW if any previous operation caused an overflow,
> not checking that the *current* operation caused an overflow.
> 

While I agree it should check the softfloat flags instead, I disagree it
is wrong. The part that GET_FP_CAUSE() is looking at is not the
accumulated flags, but the flags for the last instruction.

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