Re: [Qemu-devel] [PATCH 09/18] target-riscv: Add FMADD, FMSUB, FNMADD, FNMSUB Instructions,

[Richard Henderson]

On 09/26/2016 03:56 AM, Sagar Karandikar wrote:
> +/* convert RISC-V rounding mode to IEEE library numbers */
> +unsigned int ieee_rm[] = {
> +    float_round_nearest_even,
> +    float_round_to_zero,
> +    float_round_down,
> +    float_round_up,
> +    float_round_ties_away
> +};
> +
> +/* 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->csr[CSR_FRM];                               \
> +}                                                         \
> +if (rm > 4) {                                             \
> +    helper_raise_exception(env, RISCV_EXCP_ILLEGAL_INST); \
> +}                                                         \
> +ieee_rm[rm]; })

You're not doing anything to get the proper guest PC value for the illegal 
instruction exception.  This is, of course, rare, and can be caught during 
translation, which would be greatly preferred since the fpu signals no other 
exceptions, and thus all of the fpu helpers can be TCG_CALL_NO_RWG.

This can be done by examining fcsr during cpu_get_tb_cpu_state.  Set a flag bit 
(TB_FLAG_RM_INVALID) when fcsr.rm is invalid.  Then during translation, raise 
an exception if

(1) rm == 7 && TB_FLAG_RM_INVALID
(2) rm >= 5 && rm <= 6

You can improve this rounding mode switching by remembering the mode used by 
the previous insn.  See target-alpha DisasContext.tb_rm.

> +/* adapted from Spike's decode.h:set_fp_exceptions */
> +#define set_fp_exceptions() do { \
> +    env->csr[CSR_FFLAGS] |= 
> softfloat_flags_to_riscv(get_float_exception_flags(\
> +                            &env->fp_status)); \
> +    set_float_exception_flags(0, &env->fp_status); \
> +} while (0)

Any reason not to make this a function?

> +uint64_t helper_fmsub_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2,
> +                        uint64_t frs3, uint64_t rm)
> +{
> +    set_float_rounding_mode(RM, &env->fp_status);
> +    frs1 = float32_muladd(frs1, frs2, frs3 ^ (uint32_t)INT32_MIN, 0,
> +                          &env->fp_status);

float_muladd_negate_c

> +uint64_t helper_fnmsub_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2,
> +                         uint64_t frs3, uint64_t rm)
> +{
> +    set_float_rounding_mode(RM, &env->fp_status);
> +    frs1 = float32_muladd(frs1 ^ (uint32_t)INT32_MIN, frs2, frs3, 0,
> +                          &env->fp_status);

This one is actually computing fnmadd afaics...

> +uint64_t helper_fnmadd_s(CPURISCVState *env, uint64_t frs1, uint64_t frs2,
> +                         uint64_t frs3, uint64_t rm)
> +{
> +    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);

... and this one is actually computing fnmsub.  Given that you're passing 
tests, are you sure that FNMADD and FNMSUB opcodes are not swapped?

Use float_muladd_negate_result for FNMADD.
Use float_muladd_negate_c | float_muladd_negate_result for FNMSUB.


r~