On Tue, Jan 23, 2018 at 3:15 PM, Michael Clark <m...@sifive.com> wrote:
>
>
> On Wed, Jan 3, 2018 at 12:10 PM, Richard Henderson <
> richard.hender...@linaro.org> 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~
>>
>
>
