From: Richard Henderson <richard.henderson@linaro.org>
To: Nicholas Piggin <npiggin@gmail.com>,
Chinmay Rath <rathc@linux.ibm.com>,
qemu-ppc@nongnu.org, qemu-devel@nongnu.org
Cc: danielhb413@gmail.com, clg@kaod.org, harshpb@linux.ibm.com,
sbhat@linux.ibm.com
Subject: Re: [PATCH] target/ppc: Move floating-point arithmetic instructions to decodetree.
Date: Tue, 12 Mar 2024 04:01:07 -1000 [thread overview]
Message-ID: <0c7c6be1-12fb-4267-9d41-bd51637d104a@linaro.org> (raw)
In-Reply-To: <CZRO4Y67CBPV.2IAKB80EFDKEY@wheely>
On 3/11/24 23:36, Nicholas Piggin wrote:
> On Thu Mar 7, 2024 at 9:03 PM AEST, Chinmay Rath wrote:
>> This patch moves the below instructions to decodetree specification :
>>
>> f{add, sub, mul, div, re, rsqrte, madd, msub, nmadd, nmsub}[s][.] : A-form
>> ft{div, sqrt} : X-form
>>
>> With this patch, all the floating-point arithmetic instructions have been
>> moved to decodetree.
>> The patch also merges the definitions of different sets of helper methods of
>> the above instructions, which are similar, using macros.
>> The changes were verified by validating that the tcg ops generated by those
>> instructions remain the same, which were captured with the '-d in_asm,op' flag.
>>
>> Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
>
> Hi Chinmay,
>
> Nice patch. I think the fpu_helper.c change should be done separately.
>
>> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
>> index 4b3dcad5d1..a3fcfb3907 100644
>> --- a/target/ppc/fpu_helper.c
>> +++ b/target/ppc/fpu_helper.c
>> @@ -490,54 +490,16 @@ static void float_invalid_op_addsub(CPUPPCState *env, int flags,
>> }
>> }
>>
>> -/* fadd - fadd. */
>> -float64 helper_fadd(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64_add(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_addsub(env, flags, 1, GETPC());
>> - }
>> -
>> - return ret;
>> -}
>> -
>> -/* fadds - fadds. */
>> -float64 helper_fadds(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64r32_add(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_addsub(env, flags, 1, GETPC());
>> - }
>> - return ret;
>> -}
>> -
>> -/* fsub - fsub. */
>> -float64 helper_fsub(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64_sub(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_addsub(env, flags, 1, GETPC());
>> - }
>> -
>> - return ret;
>> -}
>> -
>> -/* fsubs - fsubs. */
>> -float64 helper_fsubs(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64r32_sub(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_addsub(env, flags, 1, GETPC());
>> - }
>> - return ret;
>> +#define FPU_ADD_SUB(name, op) \
>> +float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2) \
>> +{ \
>> + float64 ret = op(arg1, arg2, &env->fp_status); \
>> + int flags = get_float_exception_flags(&env->fp_status); \
>> + \
>> + if (unlikely(flags & float_flag_invalid)) { \
>> + float_invalid_op_addsub(env, flags, 1, GETPC()); \
>> + } \
>> + return ret; \
>> }
>>
>> static void float_invalid_op_mul(CPUPPCState *env, int flags,
>> @@ -550,29 +512,17 @@ static void float_invalid_op_mul(CPUPPCState *env, int flags,
>> }
>> }
>>
>> -/* fmul - fmul. */
>> -float64 helper_fmul(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64_mul(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_mul(env, flags, 1, GETPC());
>> - }
>> -
>> - return ret;
>> -}
>> -
>> -/* fmuls - fmuls. */
>> -float64 helper_fmuls(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64r32_mul(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_mul(env, flags, 1, GETPC());
>> - }
>> - return ret;
>> +#define FPU_MUL(name, op) \
>> +float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2) \
>> +{ \
>> + float64 ret = op(arg1, arg2, &env->fp_status); \
>> + int flags = get_float_exception_flags(&env->fp_status); \
>> + \
>> + if (unlikely(flags & float_flag_invalid)) { \
>> + float_invalid_op_mul(env, flags, 1, GETPC()); \
>> + } \
>> + \
>> + return ret; \
>> }
>>
>> static void float_invalid_op_div(CPUPPCState *env, int flags,
>> @@ -587,37 +537,31 @@ static void float_invalid_op_div(CPUPPCState *env, int flags,
>> }
>> }
>>
>> -/* fdiv - fdiv. */
>> -float64 helper_fdiv(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64_div(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_div(env, flags, 1, GETPC());
>> - }
>> - if (unlikely(flags & float_flag_divbyzero)) {
>> - float_zero_divide_excp(env, GETPC());
>> - }
>> -
>> - return ret;
>> +#define FPU_DIV(name, op) \
>> +float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2) \
>> +{ \
>> + float64 ret = op(arg1, arg2, &env->fp_status); \
>> + int flags = get_float_exception_flags(&env->fp_status); \
>> + \
>> + if (unlikely(flags & float_flag_invalid)) { \
>> + float_invalid_op_div(env, flags, 1, GETPC()); \
>> + } \
>> + if (unlikely(flags & float_flag_divbyzero)) { \
>> + float_zero_divide_excp(env, GETPC()); \
>> + } \
>> + \
>> + return ret; \
>> }
>>
>> -/* fdivs - fdivs. */
>> -float64 helper_fdivs(CPUPPCState *env, float64 arg1, float64 arg2)
>> -{
>> - float64 ret = float64r32_div(arg1, arg2, &env->fp_status);
>> - int flags = get_float_exception_flags(&env->fp_status);
>> -
>> - if (unlikely(flags & float_flag_invalid)) {
>> - float_invalid_op_div(env, flags, 1, GETPC());
>> - }
>> - if (unlikely(flags & float_flag_divbyzero)) {
>> - float_zero_divide_excp(env, GETPC());
>> - }
>>
>> - return ret;
>> -}
>> +FPU_ADD_SUB(FADD, float64_add)
>> +FPU_ADD_SUB(FADDS, float64r32_add)
>> +FPU_ADD_SUB(FSUB, float64_sub)
>> +FPU_ADD_SUB(FSUBS, float64r32_sub)
>> +FPU_MUL(FMUL, float64_mul)
>> +FPU_MUL(FMULS, float64r32_mul)
>> +FPU_DIV(FDIV, float64_div)
>> +FPU_DIV(FDIVS, float64r32_div)
>
> Several of the existing macros have slightly different styles and
> patterns. FPU_FMADD being one. You're mostly adding an existing style
> and being pretty consistent so that's fine. It would be nice if the
> others could be made more regular, but maybe not feasible.
>
> There is also quite a bit of duplication in the templates just to
> cater to different error handling. I wonder if you could consolidate
> it a bit or make it nicer if you passed in the flags handling
> function as an argument. Then you could have:
>
>
> #define FPU_HELPER(name, op, flags_handler) \
> float64 helper_##name(CPUPPCState *env, float64 arg1, float64 arg2) \
> { \
> float64 ret = op(arg1, arg2, &env->fp_status); \
> int flags = get_float_exception_flags(&env->fp_status); \
> flags_handler(env, flags) \
> return ret; \
> }
>
> static inline void addsub_flags_handler(CPUPPCState *env, int flags)
> {
> if (unlikely(flags & float_flag_invalid)) {
> float_invalid_op_addsub(env, flags, 1, GETPC());
> }
> }
>
> static inline void mul_flags_handler(CPUPPCState *env, int flags)
> {
> if (unlikely(flags & float_flag_invalid)) {
> float_invalid_op_mul(env, flags, 1, GETPC());
> }
> }
>
> static inline void div_flags_handler(CPUPPCState *env, int flags)
> {
> if (unlikely(flags & float_flag_invalid)) {
> float_invalid_op_div(env, flags, 1, GETPC());
> }
> if (unlikely(flags & float_flag_divbyzero)) {
> float_zero_divide_excp(env, GETPC());
> }
> }
Beware -- GETPC() may only be called from the outermost helper.
r~
next prev parent reply other threads:[~2024-03-12 14:02 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-07 11:03 [PATCH] target/ppc: Move floating-point arithmetic instructions to decodetree Chinmay Rath
2024-03-12 9:36 ` Nicholas Piggin
2024-03-12 14:01 ` Richard Henderson [this message]
2024-03-12 14:24 ` Nicholas Piggin
2024-03-12 14:29 ` Peter Maydell
2024-03-13 5:28 ` Nicholas Piggin
2024-03-13 11:50 ` Chinmay Rath
2024-03-12 14:45 ` Richard Henderson
2024-03-12 10:01 ` Nicholas Piggin
2024-03-13 11:52 ` Chinmay Rath
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0c7c6be1-12fb-4267-9d41-bd51637d104a@linaro.org \
--to=richard.henderson@linaro.org \
--cc=clg@kaod.org \
--cc=danielhb413@gmail.com \
--cc=harshpb@linux.ibm.com \
--cc=npiggin@gmail.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=rathc@linux.ibm.com \
--cc=sbhat@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).