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

From: Richard Henderson <richard.henderson@linaro.org>
To: Song Gao <gaosong@loongson.cn>, qemu-devel@nongnu.org
Cc: peter.maydell@linaro.org, yangxiaojuan@loongson.cn,
	david@redhat.com, bin.meng@windriver.com,
	mark.cave-ayland@ilande.co.uk, aleksandar.rikalo@syrmia.com,
	jcmvbkbc@gmail.com, tsimpson@quicinc.com,
	alistair.francis@wdc.com, edgar.iglesias@gmail.com,
	chenhuacai@gmail.com, philmd@redhat.com, atar4qemu@gmail.com,
	thuth@redhat.com, ehabkost@redhat.com, groug@kaod.org,
	maobibo@loongson.cn, mrolnik@gmail.com, shorne@gmail.com,
	alex.bennee@linaro.org, david@gibson.dropbear.id.au,
	kbastian@mail.uni-paderborn.de, crwulff@gmail.com,
	laurent@vivier.eu, palmer@dabbelt.com, pbonzini@redhat.com,
	aurelien@aurel32.net
Subject: Re: [PATCH v4 11/21] target/loongarch: Add floating point comparison instruction translation
Date: Sun, 5 Sep 2021 11:24:25 +0200	[thread overview]
Message-ID: <9481b350-9718-d8fc-11c5-69ff6f409220@linaro.org> (raw)
In-Reply-To: <1630586467-22463-12-git-send-email-gaosong@loongson.cn>

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~