From: "Nicholas Piggin" <npiggin@gmail.com>
To: "Chinmay Rath" <rathc@linux.ibm.com>, <qemu-ppc@nongnu.org>,
<qemu-devel@nongnu.org>
Cc: <danielhb413@gmail.com>, <clg@kaod.org>,
<richard.henderson@linaro.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 19:36:44 +1000 [thread overview]
Message-ID: <CZRO4Y67CBPV.2IAKB80EFDKEY@wheely> (raw)
In-Reply-To: <20240307110318.170319-1-rathc@linux.ibm.com>
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());
}
}
FPU_HELPER(FADD, float64_add, addsub_flags_handler);
FPU_HELPER(FADDS, float64r32_add, addsub_flags_handler);
FPU_HELPER(FSUB, float64_sub, addsub_flags_handler);
FPU_HELPER(FSUBS, float64r32_sub, addsub_flags_handler);
FPU_HELPER(FMUL, float64_mul, mul_flags_handler);
FPU_HELPER(FMULS, float64r32_mul, mul_flags_handler);
FPU_HELPER(FDIV, float64_div, div_flags_handler);
FPU_HELPER(FDIVS, float64r32_div, div_flags_handler);
Thoughts?
Thanks,
Nick
next prev parent reply other threads:[~2024-03-12 9:37 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 [this message]
2024-03-12 14:01 ` Richard Henderson
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=CZRO4Y67CBPV.2IAKB80EFDKEY@wheely \
--to=npiggin@gmail.com \
--cc=clg@kaod.org \
--cc=danielhb413@gmail.com \
--cc=harshpb@linux.ibm.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=rathc@linux.ibm.com \
--cc=richard.henderson@linaro.org \
--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).