Re: [Qemu-devel] [PULL 02/11] target/m68k: implement fsin

[Laurent Vivier]

Le 27/04/2018 à 15:47, Peter Maydell a écrit :
> On 13 March 2018 at 15:59, Laurent Vivier <address@hidden> wrote:
>> Using a local m68k floatx80_sin()
>> [copied from previous:
>> Written by Andreas Grabher for Previous, NeXT Computer Emulator.]
>>
>> Signed-off-by: Laurent Vivier <address@hidden>
>> Message-Id: <address@hidden>
>> ---
>>  target/m68k/fpu_helper.c |   5 +
>>  target/m68k/helper.h     |   1 +
>>  target/m68k/softfloat.c  | 247 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>  target/m68k/softfloat.h  |   1 +
>>  target/m68k/translate.c  |   3 +
>>  5 files changed, 257 insertions(+)
>>
>> diff --git a/target/m68k/fpu_helper.c b/target/m68k/fpu_helper.c
>> index c41115ea05..a6cfb4db1a 100644
>> --- a/target/m68k/fpu_helper.c
>> +++ b/target/m68k/fpu_helper.c
>> @@ -597,3 +597,8 @@ void HELPER(ftan)(CPUM68KState *env, FPReg *res, FPReg 
>> *val)
>>  {
>>      res->d = floatx80_tan(val->d, &env->fp_status);
>>  }
>> +
>> +void HELPER(fsin)(CPUM68KState *env, FPReg *res, FPReg *val)
>> +{
>> +    res->d = floatx80_sin(val->d, &env->fp_status);
>> +}
>> diff --git a/target/m68k/helper.h b/target/m68k/helper.h
>> index af9480fe49..6d8bfaab64 100644
>> --- a/target/m68k/helper.h
>> +++ b/target/m68k/helper.h
>> @@ -76,6 +76,7 @@ DEF_HELPER_3(fetox, void, env, fp, fp)
>>  DEF_HELPER_3(ftwotox, void, env, fp, fp)
>>  DEF_HELPER_3(ftentox, void, env, fp, fp)
>>  DEF_HELPER_3(ftan, void, env, fp, fp)
>> +DEF_HELPER_3(fsin, void, env, fp, fp)
>>
>>  DEF_HELPER_3(mac_move, void, env, i32, i32)
>>  DEF_HELPER_3(macmulf, i64, env, i32, i32)
>> diff --git a/target/m68k/softfloat.c b/target/m68k/softfloat.c
>> index 488dbc522b..6fb9665336 100644
>> --- a/target/m68k/softfloat.c
>> +++ b/target/m68k/softfloat.c
>> @@ -1476,3 +1476,250 @@ floatx80 floatx80_tan(floatx80 a, float_status 
>> *status)
>>          }
>>      }
>>  }
>> +
>> +/*----------------------------------------------------------------------------
>> + | Sine
>> + 
>> *----------------------------------------------------------------------------*/
>> +
>> +floatx80 floatx80_sin(floatx80 a, float_status *status)
>> +{
> 
> Hi; Coverity (CID1390617) points out that there is dead code in this function:
> 
>> +    flag aSign, xSign;
>> +    int32_t aExp, xExp;
>> +    uint64_t aSig, xSig;
>> +
>> +    int8_t user_rnd_mode, user_rnd_prec;
>> +
>> +    int32_t compact, l, n, j;
>> +    floatx80 fp0, fp1, fp2, fp3, fp4, fp5, x, invtwopi, twopi1, twopi2;
>> +    float32 posneg1, twoto63;
>> +    flag adjn, endflag;
>> +
>> +    aSig = extractFloatx80Frac(a);
>> +    aExp = extractFloatx80Exp(a);
>> +    aSign = extractFloatx80Sign(a);
>> +
>> +    if (aExp == 0x7FFF) {
>> +        if ((uint64_t) (aSig << 1)) {
>> +            return propagateFloatx80NaNOneArg(a, status);
>> +        }
>> +        float_raise(float_flag_invalid, status);
>> +        return floatx80_default_nan(status);
>> +    }
>> +
>> +    if (aExp == 0 && aSig == 0) {
>> +        return packFloatx80(aSign, 0, 0);
>> +    }
>> +
>> +    adjn = 0;
> 
> Here we set adjn == 0...
>> +
>> +    user_rnd_mode = status->float_rounding_mode;
>> +    user_rnd_prec = status->floatx80_rounding_precision;
>> +    status->float_rounding_mode = float_round_nearest_even;
>> +    status->floatx80_rounding_precision = 80;
>> +
>> +    compact = floatx80_make_compact(aExp, aSig);
>> +
>> +    fp0 = a;
>> +
>> +    if (compact < 0x3FD78000 || compact > 0x4004BC7E) {
>> +        /* 2^(-40) > |X| > 15 PI */
>> +        if (compact > 0x3FFF8000) { /* |X| >= 15 PI */
>> +            /* REDUCEX */
>> +            fp1 = packFloatx80(0, 0, 0);
>> +            if (compact == 0x7FFEFFFF) {
>> +                twopi1 = packFloatx80(aSign ^ 1, 0x7FFE,
>> +                                      LIT64(0xC90FDAA200000000));
>> +                twopi2 = packFloatx80(aSign ^ 1, 0x7FDC,
>> +                                      LIT64(0x85A308D300000000));
>> +                fp0 = floatx80_add(fp0, twopi1, status);
>> +                fp1 = fp0;
>> +                fp0 = floatx80_add(fp0, twopi2, status);
>> +                fp1 = floatx80_sub(fp1, fp0, status);
>> +                fp1 = floatx80_add(fp1, twopi2, status);
>> +            }
>> +        loop:
>> +            xSign = extractFloatx80Sign(fp0);
>> +            xExp = extractFloatx80Exp(fp0);
>> +            xExp -= 0x3FFF;
>> +            if (xExp <= 28) {
>> +                l = 0;
>> +                endflag = 1;
>> +            } else {
>> +                l = xExp - 27;
>> +                endflag = 0;
>> +            }
>> +            invtwopi = packFloatx80(0, 0x3FFE - l,
>> +                                    LIT64(0xA2F9836E4E44152A)); /* INVTWOPI 
>> */
>> +            twopi1 = packFloatx80(0, 0x3FFF + l, LIT64(0xC90FDAA200000000));
>> +            twopi2 = packFloatx80(0, 0x3FDD + l, LIT64(0x85A308D300000000));
>> +
>> +            /* SIGN(INARG)*2^63 IN SGL */
>> +            twoto63 = packFloat32(xSign, 0xBE, 0);
>> +
>> +            fp2 = floatx80_mul(fp0, invtwopi, status);
>> +            fp2 = floatx80_add(fp2, float32_to_floatx80(twoto63, status),
>> +                               status); /* THE FRACT PART OF FP2 IS ROUNDED 
>> */
>> +            fp2 = floatx80_sub(fp2, float32_to_floatx80(twoto63, status),
>> +                               status); /* FP2 is N */
>> +            fp4 = floatx80_mul(twopi1, fp2, status); /* W = N*P1 */
>> +            fp5 = floatx80_mul(twopi2, fp2, status); /* w = N*P2 */
>> +            fp3 = floatx80_add(fp4, fp5, status); /* FP3 is P */
>> +            fp4 = floatx80_sub(fp4, fp3, status); /* W-P */
>> +            fp0 = floatx80_sub(fp0, fp3, status); /* FP0 is A := R - P */
>> +            fp4 = floatx80_add(fp4, fp5, status); /* FP4 is p = (W-P)+w */
>> +            fp3 = fp0; /* FP3 is A */
>> +            fp1 = floatx80_sub(fp1, fp4, status); /* FP1 is a := r - p */
>> +            fp0 = floatx80_add(fp0, fp1, status); /* FP0 is R := A+a */
>> +
>> +            if (endflag > 0) {
>> +                n = floatx80_to_int32(fp2, status);
>> +                goto sincont;
>> +            }
>> +            fp3 = floatx80_sub(fp3, fp0, status); /* A-R */
>> +            fp1 = floatx80_add(fp1, fp3, status); /* FP1 is r := (A-R)+a */
>> +            goto loop;
>> +        } else {
>> +            /* SINSM */
>> +            fp0 = float32_to_floatx80(make_float32(0x3F800000),
>> +                                      status); /* 1 */
>> +
>> +            status->float_rounding_mode = user_rnd_mode;
>> +            status->floatx80_rounding_precision = user_rnd_prec;
>> +
>> +            if (adjn) {
> 
> ...and we never change it before here, so this branch of this 'if'
> is dead code.
> 
>> +                /* COSTINY */
>> +                a = floatx80_sub(fp0, float32_to_floatx80(
>> +                                 make_float32(0x00800000), status), status);
>> +            } else {
>> +                /* SINTINY */
>> +                a = floatx80_move(a, status);
>> +            }
>> +            float_raise(float_flag_inexact, status);
>> +
>> +            return a;
>> +        }
> 
> Similarly in floatx80_cos(), we set adjn = 1 at the top of
> the function, and then later test it.
> 
> Should these two functions be sharing code? They do look
> rather similar...

yes, I'm aware of these problems, but I let the code as-is to only port
functions from "Previous" to QEMU. In fact, the original assembly code
uses a sincos() function to compute both of them and then sin() and
cos() calls it.

All these function need some cleanup now...

Thanks,
Laurent