Re: [PATCH v2 4/4] target/m68k: fix FPSR quotient byte for frem instruction

[Mark Cave-Ayland]

On 04/01/2023 18:04, Richard Henderson wrote:

> On 1/4/23 05:45, Mark Cave-Ayland wrote:
> > The FPSR quotient byte should be set to the value of the quotient and not the
> > result. Manually calculate the quotient in the frem helper in round to nearest
> > even mode (note this is different from the quotient calculated internally for
> > fmod), and use it to set the quotient byte accordingly.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1314
> > Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> >   target/m68k/fpu_helper.c | 14 +++++++++++---
> >   1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
> > index 5fd094a33c..56f7400140 100644
> > --- a/target/m68k/fpu_helper.c
> > +++ b/target/m68k/fpu_helper.c
> > @@ -538,17 +538,25 @@ void HELPER(fmod)(CPUM68KState *env, FPReg *res, FPReg *val0, 
> > FPReg *val1)
> >   void HELPER(frem)(CPUM68KState *env, FPReg *res, FPReg *val0, FPReg *val1)
> >   {
> > +    float_status fp_status;
> > +    FPReg fp_quot;
> >       uint32_t quotient;
> >       int sign;
> > +    /* Calculate quotient directly using round to nearest mode */
> > +    set_float_rounding_mode(float_round_nearest_even, &fp_status);
> > +    set_floatx80_rounding_precision(
> > +        get_floatx80_rounding_precision(&env->fp_status), &fp_status);
> > +    fp_quot.d = floatx80_div(val1->d, val0->d, &fp_status);
> > +
> >       res->d = floatx80_rem(val1->d, val0->d, &env->fp_status);
> > -    if (floatx80_is_any_nan(res->d)) {
> > +    if (floatx80_is_any_nan(fp_quot.d)) {
>
> I think you should leave this line unchanged, and move the div afterward.
> I also think you should completely initialize the local fp_status = { }.
>
> With that,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

I can leave the floatx80_is_any_nan() line above unchanged and also initialise the 
local fp_status, however the floatx80_div() has to happen before floatx80_rem() 
function is called. This is because the fmod and frem instructions write the result 
back to one of the input registers, which then causes the subsequent floatx80_div() 
to return an incorrect result.

Would just these 2 changes be enough to keep your R-B tag for a v3?


ATB,

Mark.