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

[Song Gao <gaosong@loongson.cn>]

Hi, Richard.

On 07/23/2021 02:11 PM, Richard Henderson wrote:
> 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}.
>

Hmm, this part is seem too bad. 
 
>> +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;
>
This is a good idea!
 
Thanks
Song Gao.