Re: [PATCH 3/3] target/ppc: Merge fsqrt and fsqrts helpers

["Víctor Colombo" <victor.colombo@eldorado.org.br>]

On 05/09/2022 12:56, Richard Henderson wrote:
> On 9/5/22 13:37, Víctor Colombo wrote:
>> These two helpers are almost identical, differing only by the softfloat
>> operation it calls. Merge them into one using a macro.
>> Also, take this opportunity to capitalize the helper name as we moved
>> the instruction to decodetree in a previous patch.
>>
>> Signed-off-by: Víctor Colombo <victor.colombo@eldorado.org.br>
>> ---
>>   target/ppc/fpu_helper.c            | 35 +++++++++++-------------------
>>   target/ppc/helper.h                |  4 ++--
>>   target/ppc/translate/fp-impl.c.inc |  4 ++--
>>   3 files changed, 17 insertions(+), 26 deletions(-)
>>
>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>> index 0f045b70f8..32995179b5 100644
>> --- a/target/ppc/fpu_helper.c
>> +++ b/target/ppc/fpu_helper.c
>> @@ -830,30 +830,21 @@ static void float_invalid_op_sqrt(CPUPPCState 
>> *env, int flags,
>>       }
>>   }
>>
>> -/* fsqrt - fsqrt. */
>> -float64 helper_fsqrt(CPUPPCState *env, float64 arg)
>> -{
>> -    float64 ret = float64_sqrt(arg, &env->fp_status);
>> -    int flags = get_float_exception_flags(&env->fp_status);
>> -
>> -    if (unlikely(flags & float_flag_invalid)) {
>> -        float_invalid_op_sqrt(env, flags, 1, GETPC());
>> -    }
>> -
>> -    return ret;
>> +#define FPU_FSQRT(name, 
>> op)                                                   \
>> +float64 helper_##name(CPUPPCState *env, float64 
>> arg)                          \
>> +{                                                                             
>> \
>> +    float64 ret = op(arg, 
>> &env->fp_status);                                   \
>> +    int flags = 
>> get_float_exception_flags(&env->fp_status);                   \
>> +                                                                              
>> \
>> +    if (unlikely(flags & float_flag_invalid)) 
>> {                               \
>> +        float_invalid_op_sqrt(env, flags, 1, 
>> GETPC());                        \
>> +    
>> }                                                                         
>> \
> 
> Existing bug, but this is missing to clear fp status to start.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> r~
> 

Hello Richard, thanks for your review!
gen_reset_fpstatus() is called by the inline implementation in
do_helper_fsqrt() before calling the helper (patch 1).
It's probably better to move the call to inside the helper.

>> +                                                                              
>> \
>> +    return 
>> ret;                                                               \
>>   }
>>
>> -/* fsqrts - fsqrts. */
>> -float64 helper_fsqrts(CPUPPCState *env, float64 arg)
>> -{
>> -    float64 ret = float64r32_sqrt(arg, &env->fp_status);
>> -    int flags = get_float_exception_flags(&env->fp_status);
>> -
>> -    if (unlikely(flags & float_flag_invalid)) {
>> -        float_invalid_op_sqrt(env, flags, 1, GETPC());
>> -    }
>> -    return ret;
>> -}
>> +FPU_FSQRT(FSQRT, float64_sqrt)
>> +FPU_FSQRT(FSQRTS, float64r32_sqrt)
>>
>>   /* fre - fre. */
>>   float64 helper_fre(CPUPPCState *env, float64 arg)
>> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
>> index 159b352f6e..68610896b8 100644
>> --- a/target/ppc/helper.h
>> +++ b/target/ppc/helper.h
>> @@ -116,8 +116,8 @@ DEF_HELPER_4(fmadds, i64, env, i64, i64, i64)
>>   DEF_HELPER_4(fmsubs, i64, env, i64, i64, i64)
>>   DEF_HELPER_4(fnmadds, i64, env, i64, i64, i64)
>>   DEF_HELPER_4(fnmsubs, i64, env, i64, i64, i64)
>> -DEF_HELPER_2(fsqrt, f64, env, f64)
>> -DEF_HELPER_2(fsqrts, f64, env, f64)
>> +DEF_HELPER_2(FSQRT, f64, env, f64)
>> +DEF_HELPER_2(FSQRTS, f64, env, f64)
>>   DEF_HELPER_2(fre, i64, env, i64)
>>   DEF_HELPER_2(fres, i64, env, i64)
>>   DEF_HELPER_2(frsqrte, i64, env, i64)
>> diff --git a/target/ppc/translate/fp-impl.c.inc 
>> b/target/ppc/translate/fp-impl.c.inc
>> index 7a90c0e350..8d5cf0f982 100644
>> --- a/target/ppc/translate/fp-impl.c.inc
>> +++ b/target/ppc/translate/fp-impl.c.inc
>> @@ -280,8 +280,8 @@ static bool do_helper_fsqrt(DisasContext *ctx, 
>> arg_A_tb *a,
>>       return true;
>>   }
>>
>> -TRANS(FSQRT, do_helper_fsqrt, gen_helper_fsqrt);
>> -TRANS(FSQRTS, do_helper_fsqrt, gen_helper_fsqrts);
>> +TRANS(FSQRT, do_helper_fsqrt, gen_helper_FSQRT);
>> +TRANS(FSQRTS, do_helper_fsqrt, gen_helper_FSQRTS);
>>
>>   /***                     Floating-Point 
>> multiply-and-add                   ***/
>>   /* fmadd - fmadds */
> 


-- 
Víctor Cora Colombo
Instituto de Pesquisas ELDORADO
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>