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~