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

[Michael Clark]

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_float_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/4223d89b0c5c671332d66bcd649db5c6f46559f5


> > +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.


> > +/* 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~
>