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

[Christoph Müllner]

On Mon, Mar 27, 2023 at 7:18 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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.

I don't see how this can help:
* default_nan_mode defines if the default_nan is generated or if the
operand's NaN should be used
* RISC-V has default_nan_mode always set to true (operations should
return the a canonical NaN and not propagate NaN values)
* That also does not help to eliminate the is_any_nan() tests, because
float*_minimum_number() and float*_minnum() return the non-NaN number
if (only) one operand is NaN

Am I missing something?


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

Understood the issue and changed to the proposed API.

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

Done.

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

Understood the issue.
ARM has something similar in HELPER(fjcvtzs).

Given the different flag behaviour of the ARM and the Alpha
instructions, I created a RISC-V specific routine.
For RISC-V the flags have to be identical to fcvt.w.d with the same value.

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

Fixed.

Thanks!