Re: [PATCH v4 10/21] target/loongarch: Add floating point arithmetic instruction translation

[Richard Henderson <richard.henderson@linaro.org>]

On 9/2/21 2:40 PM, Song Gao wrote:
> +const FloatRoundMode ieee_rm[4] = {

Make static.

> +int ieee_ex_to_loongarch(int xcpt)

This function is only used in this file.  Better to make it static and remove the 
declaration from translate.h, which allows you to remove that include.

> +static inline void update_fcsr0(CPULoongArchState *env, uintptr_t pc)

Remove inline.

> +{
> +    int flags = ieee_ex_to_loongarch(get_float_exception_flags(
> +                                  &env->fp_status));
> +
> +    SET_FP_CAUSE(env->fcsr0, flags);
> +    if (flags) {

Hmm.  Between the two functions you're testing for zero twice.  Perhaps better to rearrange as

{
     int flags = get_float_exception_flags(&env->fp_status);

     if (!flags) {
         SET_FP_CAUSE(env->fcsr0, 0);
         return;
     }

     set_float_exception_flags(0, &env->fp_status);
     flags = ieee_ex_to_loongarch(flags);
     SET_FP_CAUSE(env->fcsr0, flags);

     if (GET_FP_ENABLE(env->fcsr0) & flags) {
         do_raise_exception(env, EXCP_FPE, pc);
     }
     UPDATE_FP_FLAGS(env->fcsr0, flags);
}

and remove the explicit zero test in ieee_ex_to_loongarch.

> +uint64_t helper_flogb_s(CPULoongArchState *env, uint64_t fj)
> +{
> +    uint64_t fd;
> +    uint32_t fp;
> +    float_status *status = &env->fp_status;
> +    FloatRoundMode old_mode = get_float_rounding_mode(status);
> +
> +    set_float_exception_flags(0, status);

Unnecessary, since the steady-state is already zero.

> +    set_float_exception_flags(get_float_exception_flags(status) &
> +                              (~float_flag_inexact), status);
> +    update_fcsr0(env, GETPC());

Hmm.  Worth adding a parameter to update_fcsr0, a mask to suppress?  Or a common 
subroutine like

     update_fcsr0_mask(env, GETPC(), float_flag_inexact);

static void update_fcsr0(env, ra)
{
     update_fcsr0_mask(env, ra, 0);
}

> +static bool trans_fcopysign_s(DisasContext *ctx, arg_fmt_fdfjfk *a)
> +{
> +    tcg_gen_deposit_i64(cpu_fpr[a->fd], cpu_fpr[a->fk], cpu_fpr[a->fj], 0, 31);
> +    return true;
> +}
> +
> +static bool trans_fcopysign_d(DisasContext *ctx, arg_fmt_fdfjfk *a)
> +{
> +    tcg_gen_deposit_i64(cpu_fpr[a->fd], cpu_fpr[a->fk], cpu_fpr[a->fj], 0, 63);
> +    return true;
> +}

For what it's worth, you could treat the abs and neg instructions similarly, inline.


r~