[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 14/22] target/loongarch: Add floating point comparison ins
From: |
Song Gao |
Subject: |
Re: [PATCH v2 14/22] target/loongarch: Add floating point comparison instruction translation |
Date: |
Tue, 27 Jul 2021 15:56:38 +0800 |
User-agent: |
Mozilla/5.0 (X11; Linux mips64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
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.
- Re: [PATCH v2 12/22] target/loongarch: Add fixed point extra instruction translation, (continued)
[PATCH v2 13/22] target/loongarch: Add floating point arithmetic instruction translation, Song Gao, 2021/07/21
[PATCH v2 15/22] target/loongarch: Add floating point conversion instruction translation, Song Gao, 2021/07/21
[PATCH v2 14/22] target/loongarch: Add floating point comparison instruction translation, Song Gao, 2021/07/21
[PATCH v2 16/22] target/loongarch: Add floating point move instruction translation, Song Gao, 2021/07/21
[PATCH v2 18/22] target/loongarch: Add branch instruction translation, Song Gao, 2021/07/21
[PATCH v2 17/22] target/loongarch: Add floating point load/store instruction translation, Song Gao, 2021/07/21
[PATCH v2 19/22] target/loongarch: Add disassembler, Song Gao, 2021/07/21