Re: [PATCH v4 11/21] target/loongarch: Add floating point comparison instruction translation

[Richard Henderson]

On 9/2/21 2:40 PM, Song Gao wrote:
> +/* fcmp_cXXX_s */
> +uint64_t helper_fcmp_c_s(CPULoongArchState *env, uint64_t fj,
> +                         uint64_t fk, uint32_t flags)
> +{
> +    uint32_t t0, t1;
> +    uint64_t cmp = 0;
> +    t0 = (uint32_t)fj;
> +    t1 = (uint32_t)fk;
> +
> +    if (flags) {
> +        if (flags & FCMP_LT) {
> +            cmp |= float32_lt_quiet(t0, t1, &env->fp_status);
> +        }
> +        if (flags & FCMP_EQ) {
> +            cmp |= float32_eq_quiet(t0, t1, &env->fp_status);
> +        }
> +        if (flags & FCMP_GT) {
> +            cmp |= float32_lt_quiet(t1, t0, &env->fp_status);
> +        }
> +        if (flags & FCMP_UN) {
> +            cmp |= float32_unordered_quiet(t1, t0, &env->fp_status);
> +        }
> +    } else {
> +        cmp = (float32_unordered_quiet(t1, t0, &env->fp_status), 0);
> +    }

This is silly, especially the form of the last.  You want

    FloatRelation cmp = float32_unordered_quiet(t1, t0, &env->fp_status);
    bool ret;

    switch (cmp) {
    case float_relation_less:
        ret = (flags & FCMP_LT);
        break;
    case float_relation_equal:
        ret = (flags & FCMP_EQ);
        break;
    case float_relation_greater:
        ret = (flags & FCMP_GT);
        break;
    case float_relation_unordered:
        ret = (flags & FCMP_UN);
        break;
    default:
        g_assert_not_reached();
    }
    return ret;

And of course the switch can be split out to a common subroutine for use with the other 3 
comparison helpers.

> +static bool trans_fcmp_cond_s(DisasContext *ctx, arg_fcmp_cond_s *a)
> +{
> +    TCGv var = tcg_temp_new();
> +    TCGv_i32 flags = NULL;
> +
> +    switch (a->fcond) {
> +    /* caf */
> +    case  0:
> +        flags = tcg_constant_i32(0);
> +        gen_helper_fcmp_c_s(var, cpu_env, cpu_fpr[a->fj],
> +                            cpu_fpr[a->fk], flags);
> +        break;
> +    /* saf */
> +    case 1:
> +        flags = tcg_constant_i32(0);
> +        gen_helper_fcmp_s_s(var, cpu_env, cpu_fpr[a->fj],
> +                            cpu_fpr[a->fk], flags);
> +        break;

Hmm.  Perhaps put the integer flags into a table?

    fn = (a->fcond & 1 ? gen_helper_fcmp_s_s : gen_helper_fcmp_c_s);
    flags = fcond_table[a->fcond >> 1];

    fn(var, cpu_env, cpu_fpr[a->fj], cpu_fpr[a->fk],
       tcg_constant_i32(flags));

> +static bool trans_fcmp_cond_d(DisasContext *ctx, arg_fcmp_cond_d *a)

You'd get to share the table with this function.

> --- a/target/loongarch/translate.h
> +++ b/target/loongarch/translate.h
> @@ -11,6 +11,11 @@
>   
>   #include "exec/translator.h"
>   
> +#define FCMP_LT   0x0001  /* fp0 < fp1 */
> +#define FCMP_EQ   0x0010  /* fp0 = fp1 */
> +#define FCMP_GT   0x0100  /* fp1 < fp0 */
> +#define FCMP_UN   0x1000  /* unordered */

Hmm.  Better in internals.h?  I don't think you want to include translate.h 
into fpu_helper.c.


r~