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.
