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

[Richard Henderson]

On 1/7/23 15:00, Mark Cave-Ayland wrote:
> > >   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?

Mm.  I suppose so.  Otherwise, compute into a local variable:

    floatx80 fp_rem = floatx80_rem(val1->d, val0->d, &env->fp_status);
    if (!floatx80_is_any_nan(fp_rem)) {
        float_status scratch = env->fp_status;
        floatx80 fp_quot;
        uint32_t int_quot;
        int sign;

        set_float_rounding_mode(float_round_nearest_even, &scratch);
        fp_quot = floatx80_div(val1->d, val0->d, &scratch);

        sign = ...
        int_quot = ...
        ...
    }
    res->d = fp_rem;

?


r~