Re: [PATCH] riscv: Add support for the Zfa extension

[Richard Henderson]

On 3/27/23 01:00, Christoph Muellner wrote:
> +uint64_t helper_fminm_s(CPURISCVState *env, uint64_t rs1, uint64_t rs2)
> +{
> +    float32 frs1 = check_nanbox_s(env, rs1);
> +    float32 frs2 = check_nanbox_s(env, rs2);
> +
> +    if (float32_is_any_nan(frs1) || float32_is_any_nan(frs2)) {
> +        return float32_default_nan(&env->fp_status);
> +    }
> +
> +    return nanbox_s(env, float32_minimum_number(frs1, frs2, &env->fp_status));
> +}

Better to set and clear fp_status->default_nan_mode around the operation.

> +uint64_t helper_fround_s(CPURISCVState *env, uint64_t frs1)
> +{
> +    if (float32_is_zero(frs1) ||
> +        float32_is_infinity(frs1)) {
> +        return frs1;
> +    }
> +
> +    if (float32_is_any_nan(frs1)) {
> +        riscv_cpu_set_fflags(env, FPEXC_NV);
> +        return frs1;
> +    }
> +
> +    int32_t tmp = float32_to_int32(frs1, &env->fp_status);
> +    return nanbox_s(env, int32_to_float32(tmp, &env->fp_status));
> +}

Very much incorrect, since int32_t does not have the range for the intermediate result. 
In any case, the function you want is float32_round_to_int, which eliminates the 
zero/inf/nan special cases.  It will raise inexact, so perfect for froundnx, but you'll 
need to save/restore float_flag_inexact around the function.

> +uint64_t helper_fli_s(CPURISCVState *env, uint32_t rs1)
> +{
> +    const uint32_t fli_s_table[] = {

static const.  You don't need to use float32_default_nan, use the correct architected 
constant.  This entire operation should be done at translation time.

> +target_ulong helper_fcvtmod_w_d(CPURISCVState *env, uint64_t frs1)
> +{
> +    if (float64_is_any_nan(frs1) ||
> +        float64_is_infinity(frs1)) {
> +        return 0;
> +    }
> +
> +    return float64_to_int32(frs1, &env->fp_status);
> +}

Incorrect, as float64_to_int32 will saturate the result, whereas you need the 
modular result.

There is code to do the conversion mod 2**64 in target/alpha/ (do_cvttq).  We should move 
this to generic code if it is to be used by more than one target.

> +bool trans_fmvp_d_x(DisasContext *ctx, arg_fmvp_d_x *a)
> +{
> +    REQUIRE_FPU;
> +    REQUIRE_ZFA(ctx);
> +    REQUIRE_EXT(ctx, RVD);
> +    REQUIRE_32BIT(ctx);
> +
> +    TCGv src1 = get_gpr(ctx, a->rs1, EXT_ZERO);
> +    TCGv_i64 t1 = tcg_temp_new_i64();
> +
> +    tcg_gen_extu_tl_i64(t1, src1);
> +    tcg_gen_deposit_i64(cpu_fpr[a->rd], cpu_fpr[a->rd], t1, 32, 32);
> +    mark_fs_dirty(ctx);
> +    return true;
> +}

This does not match the linked document, which says that this insn has two inputs and sets 
the complete fpr.


r~