Re: [PATCH v2 14/22] target/loongarch: Add floating point comparison instruction translation

[Richard Henderson]

On 7/20/21 11:53 PM, Song Gao wrote:
> +void helper_movreg2cf_i32(CPULoongArchState *env, uint32_t cd, uint32_t src)
> +{
> +    env->active_fpu.cf[cd & 0x7] = src & 0x1;
> +}
> +
> +void helper_movreg2cf_i64(CPULoongArchState *env, uint32_t cd, uint64_t src)
> +{
> +    env->active_fpu.cf[cd & 0x7] = src & 0x1;
> +}
> +
> +/* fcmp.cond.s */
> +uint32_t helper_fp_cmp_caf_s(CPULoongArchState *env, uint32_t fp,
> +                             uint32_t fp1)
> +{
> +    uint64_t ret;
> +    ret = (float32_unordered_quiet(fp1, fp, &env->active_fpu.fp_status), 0);
> +    update_fcsr0(env, GETPC());
> +    if (ret) {
> +        return -1;
> +    } else {
> +        return 0;
> +    }
> +}

I don't understand why you have split the compare from the store to cf?

I don't understand why you're returning -1 instead of 1, when the result is supposed to be 
a boolean.

Alternately, I don't understand why you want a helper function to perform a simple byte 
store operation.  You could easily store a byte with tcg_gen_st8_{i32,i64}.

> +uint32_t helper_fp_cmp_cueq_s(CPULoongArchState *env, uint32_t fp,
> +                              uint32_t fp1)
> +{
> +    uint64_t ret;
> +    ret = float32_unordered_quiet(fp1, fp, &env->active_fpu.fp_status) ||
> +          float32_eq_quiet(fp, fp1, &env->active_fpu.fp_status);

You're better off using

    FloatRelation cmp = float32_compare_quiet(fp0, fp1, status);
    update_fcsr0(env, GETPC();
    return cmp == float_relation_unordered ||
           cmp == float_relation_equal;

Similarly with every other place you use two comparisons.

Indeed, one could conceivably condense everything into exactly four helper functions: two 
using float{32,64}_compare_quiet and two using float{32,64}_compare (signalling).  A 4th 
argument would be a bitmask of the different true conditions, exactly as listed in Table 9.

Since FloatRelation is in {-1, 0, 1, 2}, one could write

  return (mask >> (cmp + 1)) & 1;


r~