Re: [Qemu-devel] [PATCH v1 06/21] RISC-V FPU Support

[Michael Clark]

On Tue, Jan 23, 2018 at 3:15 PM, Michael Clark <address@hidden> wrote:

>
>
> On Wed, Jan 3, 2018 at 12:10 PM, Richard Henderson <
> address@hidden> wrote:
>
>> On 01/02/2018 04:44 PM, Michael Clark wrote:
>> > +/* convert RISC-V rounding mode to IEEE library numbers */
>> > +unsigned int ieee_rm[] = {
>>
>> static const.
>>
>> > +/* obtain rm value to use in computation
>> > + * as the last step, convert rm codes to what the softfloat library
>> expects
>> > + * Adapted from Spike's decode.h:RM
>> > + */
>> > +#define RM ({                                             \
>> > +if (rm == 7) {                                            \
>> > +    rm = env->frm;                               \
>> > +}                                                         \
>> > +if (rm > 4) {                                             \
>> > +    helper_raise_exception(env, RISCV_EXCP_ILLEGAL_INST); \
>> > +}                                                         \
>> > +ieee_rm[rm]; })
>>
>> Please use inline functions and not these macros.
>>
>> Probably this applies to some of the helpers that I've already reviewed,
>> but
>> you're going to need to use an exception raising function that also
>> performs an
>> unwind (usually via cpu_loop_exit_restore).  The GETPC() that feeds the
>> unwind
>> must be placed in the outer-most helper (including inlines).
>>
>> > +#ifndef CONFIG_USER_ONLY
>> > +#define require_fp if (!(env->mstatus & MSTATUS_FS)) { \
>> > +    helper_raise_exception(env, RISCV_EXCP_ILLEGAL_INST); \
>> > +}
>>
>> If you included MSTATUS_FS in cpu_get_tb_cpu_state flags, then you could
>> be
>> checking for this at translation time instead of at run time.
>>
>> > +/* convert softfloat library flag numbers to RISC-V */
>> > +unsigned int softfloat_flags_to_riscv(unsigned int flags)
>> > +{
>> > +    int rv_flags = 0;
>> > +    rv_flags |= (flags & float_flag_inexact) ? 1 : 0;
>> > +    rv_flags |= (flags & float_flag_underflow) ? 2 : 0;
>> > +    rv_flags |= (flags & float_flag_overflow) ? 4 : 0;
>> > +    rv_flags |= (flags & float_flag_divbyzero) ? 8 : 0;
>> > +    rv_flags |= (flags & float_flag_invalid) ? 16 : 0;
>>
>> FPEXC_NX et al.
>>
>> > +/* adapted from Spike's decode.h:set_fp_exceptions */
>> > +#define set_fp_exceptions() do { \
>> > +    env->fflags |= softfloat_flags_to_riscv(get_f
>> loat_exception_flags(\
>> > +                            &env->fp_status)); \
>> > +    set_float_exception_flags(0, &env->fp_status); \
>> > +} while (0)
>>
>> inline function.  Usually written as
>>
>>   int flags = get_float_exception_flags(&env->fp_status);
>>   if (flags) {
>>     set_float_exception_flags(0, &env->fp_status);
>>     env->fflags |= softfloat_flags_to_riscv(flags);
>>   }
>>
>> since we really do expect exceptions to be exceptional.
>>
>> > +uint64_t helper_fmsub_s(CPURISCVState *env, uint64_t frs1, uint64_t
>> frs2,
>> > +                        uint64_t frs3, uint64_t rm)
>> > +{
>> > +    require_fp;
>> > +    set_float_rounding_mode(RM, &env->fp_status);
>> > +    frs1 = float32_muladd(frs1, frs2, frs3 ^ (uint32_t)INT32_MIN, 0,
>> > +                          &env->fp_status);
>>
>> Given that RISC-V always returns a default NaN, you obviously do not care
>> about
>> the sign of a NaN result.  Therefore you should use float_muladd_negate_c
>> as
>> the fourth argument here and not perform the sign flip manually.
>
>
> We do care about the sign of NaN results.
>
> Jim Wilson spotted this bug and removed a call to set_default_nan_mode
>
> https://github.com/riscv/riscv-qemu/commit/4223d89b0c5c671332d66bcd649db5
> c6f46559f5
>
>
>> > +uint64_t helper_fnmsub_s(CPURISCVState *env, uint64_t frs1, uint64_t
>> frs2,
>> > +                         uint64_t frs3, uint64_t rm)
>> > +{
>> > +    require_fp;
>> > +    set_float_rounding_mode(RM, &env->fp_status);
>> > +    frs1 = float32_muladd(frs1 ^ (uint32_t)INT32_MIN, frs2, frs3, 0,
>> > +                          &env->fp_status);
>>
>> float_muladd_negate_product.
>>
>> > +uint64_t helper_fnmadd_s(CPURISCVState *env, uint64_t frs1, uint64_t
>> frs2,
>> > +                         uint64_t frs3, uint64_t rm)
>> > +{
>> > +    require_fp;
>> > +    set_float_rounding_mode(RM, &env->fp_status);
>> > +    frs1 = float32_muladd(frs1 ^ (uint32_t)INT32_MIN, frs2,
>> > +                          frs3 ^ (uint32_t)INT32_MIN, 0,
>> &env->fp_status);
>>
>> float_muladd_negate_c | float_muladd_negate_product
>>
>> > +uint64_t helper_fmin_s(CPURISCVState *env, uint64_t frs1, uint64_t
>> frs2)
>> > +{
>> > +    require_fp;
>> > +    frs1 = float32_minnum(frs1, frs2, &env->fp_status);
>>
>> If you want minnum and not min, riscv-spec-v2.2.pdf could use some more
>> verbage
>> to specify that.
>
>
> We'll look at Spike (riscv-isa-sim)... Spike has the correct behavior.
>

We want minimum number (minnum). It's been added to the draft spec and will
be in riscv-spec-v2.3.pdf

Section 8.6 in the draft spec says:

"
Floating-point minimum-number and maximum-number instructions FMIN.S and
FMAX.S write, respectively, the smaller or larger of rs1 and rs2 to rd. For
the purposes of these instructions only, the value -0:0 is considered to be
less than the value +0:0. If both inputs are NaNs, the result is the
canonical NaN. If only one operand is a NaN, the result is the non-NaN
operand. Signaling NaN inputs raise the invalid operation exception, even
when the result is not NaN.
"


> +/* adapted from spike */
>> > +#define isNaNF32UI(ui) (0xFF000000 < (uint32_t)((uint_fast32_t)ui <<
>> 1))
>>
>> float32_is_any_nan
>>
>> > +#define signF32UI(a) ((bool)((uint32_t)a >> 31))
>>
>> float32_is_neg
>>
>> > +#define expF32UI(a) ((int_fast16_t)(a >> 23) & 0xFF)
>> > +#define fracF32UI(a) (a & 0x007FFFFF)
>>
>> Either float32_is_infinity or float32_is_zero_or_denormal.
>>
>> > +union ui32_f32 { uint32_t ui; uint32_t f; };
>>
>> This is just silly.
>>
>> > +uint_fast16_t float32_classify(uint32_t a, float_status *status)
>>
>> Please drop *_fast*_t for just "int".
>>
>> > +    return
>> > +        (sign && infOrNaN && fracF32UI(uiA) == 0)           << 0 |
>> > +        (sign && !infOrNaN && !subnormalOrZero)             << 1 |
>> > +        (sign && subnormalOrZero && fracF32UI(uiA))         << 2 |
>> > +        (sign && subnormalOrZero && fracF32UI(uiA) == 0)    << 3 |
>> > +        (!sign && infOrNaN && fracF32UI(uiA) == 0)          << 7 |
>> > +        (!sign && !infOrNaN && !subnormalOrZero)            << 6 |
>> > +        (!sign && subnormalOrZero && fracF32UI(uiA))        << 5 |
>> > +        (!sign && subnormalOrZero && fracF32UI(uiA) == 0)   << 4 |
>> > +        (isNaNF32UI(uiA) &&  float32_is_signaling_nan(uiA, status)) <<
>> 8 |
>> > +        (isNaNF32UI(uiA) && !float32_is_signaling_nan(uiA, status)) <<
>> 9;
>>
>> These conditions are all exclusive.  You do not need to compute all of
>> them.
>>
>>   if (float32_is_any_nan(f)) {
>>     if (float32_is_signaling_nan(f, status)) {
>>       return 1 << 8;
>>     } else {
>>       return 1 << 9;
>>     }
>>   }
>>   if (float32_is_neg(f)) {
>>     if (float32_is_infinity(f)) {
>>       return 1 << 0;
>>     } else if (float32_is_zero(f)) {
>>       return 1 << 3;
>>     } else if (float32_is_zero_or_denormal(f)) {
>>       return 1 << 2;
>>     } else {
>>       return 1 << 1;
>>     }
>>   } else {
>>     ...
>>   }
>>
>> > +    frs1 = (int64_t)((int32_t)float64_to_int32(frs1,
>> &env->fp_status));
>>
>> Double cast is pointless, as are the extra parenthesis.
>>
>> > +/* adapted from spike */
>> > +#define isNaNF64UI(ui) (UINT64_C(0xFFE0000000000000) \
>> > +                       < (uint64_t)((uint_fast64_t)ui << 1))
>> > +#define signF64UI(a) ((bool)((uint64_t) a >> 63))
>> > +#define expF64UI(a) ((int_fast16_t)(a >> 52) & 0x7FF)
>> > +#define fracF64UI(a) (a & UINT64_C(0x000FFFFFFFFFFFFF))
>> > +
>> > +union ui64_f64 { uint64_t ui; uint64_t f; };
>> > +
>> > +uint_fast16_t float64_classify(uint64_t a, float_status *status)
>>
>> Likewise.
>>
>>
>> r~
>>
>
>