diff mbox series

[v2,06/12] target/ppc: implement xscvspdpn with helper_todouble

Message ID 20220519201822.465229-7-matheus.ferst@eldorado.org.br
State Accepted, archived
Headers show
Series Change helper declarations to use call flags | expand

Commit Message

Matheus K. Ferst May 19, 2022, 8:18 p.m. UTC 
From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Move xscvspdpn to decodetree, drop helper_xscvspdpn and use
helper_todouble directly.

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/fpu_helper.c             |  5 -----
 target/ppc/helper.h                 |  1 -
 target/ppc/insn32.decode            |  1 +
 target/ppc/translate/vsx-impl.c.inc | 26 +++++++++++++++++++++++++-
 target/ppc/translate/vsx-ops.c.inc  |  1 -
 5 files changed, 26 insertions(+), 8 deletions(-)

Comments

Richard Henderson May 20, 2022, 3:28 p.m. UTC | #1 
On 5/19/22 13:18, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst<matheus.ferst@eldorado.org.br>
> 
> Move xscvspdpn to decodetree, drop helper_xscvspdpn and use
> helper_todouble directly.
> 
> Signed-off-by: Matheus Ferst<matheus.ferst@eldorado.org.br>
> ---
>   target/ppc/fpu_helper.c             |  5 -----
>   target/ppc/helper.h                 |  1 -
>   target/ppc/insn32.decode            |  1 +
>   target/ppc/translate/vsx-impl.c.inc | 26 +++++++++++++++++++++++++-
>   target/ppc/translate/vsx-ops.c.inc  |  1 -
>   5 files changed, 26 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~
Daniel Henrique Barboza May 23, 2022, 1:48 p.m. UTC | #2 
./scripts/checkpatch.pl is complaining about something that I don't
agree with:


On 5/19/22 17:18, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Move xscvspdpn to decodetree, drop helper_xscvspdpn and use
> helper_todouble directly.
> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>   target/ppc/fpu_helper.c             |  5 -----
>   target/ppc/helper.h                 |  1 -
>   target/ppc/insn32.decode            |  1 +
>   target/ppc/translate/vsx-impl.c.inc | 26 +++++++++++++++++++++++++-
>   target/ppc/translate/vsx-ops.c.inc  |  1 -
>   5 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
> index b4d6f6ed4c..9bde333006 100644
> --- a/target/ppc/fpu_helper.c
> +++ b/target/ppc/fpu_helper.c
> @@ -2875,11 +2875,6 @@ uint64_t helper_xscvdpspn(CPUPPCState *env, uint64_t xb)
>       return (result << 32) | result;
>   }
>   
> -uint64_t helper_xscvspdpn(CPUPPCState *env, uint64_t xb)
> -{
> -    return helper_todouble(xb >> 32);
> -}
> -
>   /*
>    * VSX_CVT_FP_TO_INT - VSX floating point to integer conversion
>    *   op    - instruction mnemonic
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 4a7cbdf922..5cee55176b 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -395,7 +395,6 @@ DEF_HELPER_3(XSCVSQQP, void, env, vsr, vsr)
>   DEF_HELPER_3(xscvhpdp, void, env, vsr, vsr)
>   DEF_HELPER_4(xscvsdqp, void, env, i32, vsr, vsr)
>   DEF_HELPER_3(xscvspdp, void, env, vsr, vsr)
> -DEF_HELPER_2(xscvspdpn, i64, env, i64)
>   DEF_HELPER_3(xscvdpsxds, void, env, vsr, vsr)
>   DEF_HELPER_3(xscvdpsxws, void, env, vsr, vsr)
>   DEF_HELPER_3(xscvdpuxds, void, env, vsr, vsr)
> diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
> index 1d0b55bde3..d4c2615b1a 100644
> --- a/target/ppc/insn32.decode
> +++ b/target/ppc/insn32.decode
> @@ -708,6 +708,7 @@ XSCVUQQP        111111 ..... 00011 ..... 1101000100 -   @X_tb
>   XSCVSQQP        111111 ..... 01011 ..... 1101000100 -   @X_tb
>   XVCVBF16SPN     111100 ..... 10000 ..... 111011011 ..   @XX2
>   XVCVSPBF16      111100 ..... 10001 ..... 111011011 ..   @XX2
> +XSCVSPDPN       111100 ..... ----- ..... 101001011 ..   @XX2
>   
>   ## VSX Vector Test Least-Significant Bit by Byte Instruction
>   
> diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
> index 3692740736..cc0601a14e 100644
> --- a/target/ppc/translate/vsx-impl.c.inc
> +++ b/target/ppc/translate/vsx-impl.c.inc
> @@ -1045,7 +1045,31 @@ GEN_VSX_HELPER_R2(xscvqpuwz, 0x04, 0x1A, 0x01, PPC2_ISA300)
>   GEN_VSX_HELPER_X2(xscvhpdp, 0x16, 0x15, 0x10, PPC2_ISA300)
>   GEN_VSX_HELPER_R2(xscvsdqp, 0x04, 0x1A, 0x0A, PPC2_ISA300)
>   GEN_VSX_HELPER_X2(xscvspdp, 0x12, 0x14, 0, PPC2_VSX)
> -GEN_VSX_HELPER_XT_XB_ENV(xscvspdpn, 0x16, 0x14, 0, PPC2_VSX207)
> +
> +bool trans_XSCVSPDPN(DisasContext *ctx, arg_XX2 *a)
> +{

^ here


Checking 0006-target-ppc-declare-xscvspdpn-helper-with-call-flags.patch...
ERROR: spaces required around that '*' (ctx:WxV)
#69: FILE: target/ppc/translate/vsx-impl.c.inc:1049:
+bool trans_XSCVSPDPN(DisasContext *ctx, arg_XX2 *a)
                                                  ^

My guess is that since the var 'arg_XX2' ends with a numeral the script
thinks that the following '*' is an arithmetic operation. Problem is that
we have other examples of this kind of declaration in the same file, e.g.:


static bool trans_XVCVBF16SPN(DisasContext *ctx, arg_XX2 *a)



Is there a way to convince checkpatch.pl that this is an okay format?



Thanks,


Daniel




> +    TCGv_i64 t;
> +    TCGv_i32 b;
> +
> +    REQUIRE_INSNS_FLAGS2(ctx, VSX207);
> +    REQUIRE_VSX(ctx);
> +
> +    t = tcg_temp_new_i64();
> +    b = tcg_temp_new_i32();
> +
> +    tcg_gen_ld_i32(b, cpu_env, offsetof(CPUPPCState, vsr[a->xb].VsrW(0)));
> +
> +    gen_helper_todouble(t, b);
> +
> +    set_cpu_vsr(a->xt, t, true);
> +    set_cpu_vsr(a->xt, tcg_constant_i64(0), false);
> +
> +    tcg_temp_free_i64(t);
> +    tcg_temp_free_i32(b);
> +
> +    return true;
> +}
> +
>   GEN_VSX_HELPER_X2(xscvdpsxds, 0x10, 0x15, 0, PPC2_VSX)
>   GEN_VSX_HELPER_X2(xscvdpsxws, 0x10, 0x05, 0, PPC2_VSX)
>   GEN_VSX_HELPER_X2(xscvdpuxds, 0x10, 0x14, 0, PPC2_VSX)
> diff --git a/target/ppc/translate/vsx-ops.c.inc b/target/ppc/translate/vsx-ops.c.inc
> index b8fd116728..52d7ab30cd 100644
> --- a/target/ppc/translate/vsx-ops.c.inc
> +++ b/target/ppc/translate/vsx-ops.c.inc
> @@ -200,7 +200,6 @@ GEN_XX2FORM(xscvdpspn, 0x16, 0x10, PPC2_VSX207),
>   GEN_XX2FORM_EO(xscvhpdp, 0x16, 0x15, 0x10, PPC2_ISA300),
>   GEN_VSX_XFORM_300_EO(xscvsdqp, 0x04, 0x1A, 0x0A, 0x00000001),
>   GEN_XX2FORM(xscvspdp, 0x12, 0x14, PPC2_VSX),
> -GEN_XX2FORM(xscvspdpn, 0x16, 0x14, PPC2_VSX207),
>   GEN_XX2FORM(xscvdpsxds, 0x10, 0x15, PPC2_VSX),
>   GEN_XX2FORM(xscvdpsxws, 0x10, 0x05, PPC2_VSX),
>   GEN_XX2FORM(xscvdpuxds, 0x10, 0x14, PPC2_VSX),
Richard Henderson May 23, 2022, 3:54 p.m. UTC | #3 
On 5/23/22 06:48, Daniel Henrique Barboza wrote:
> Checking 0006-target-ppc-declare-xscvspdpn-helper-with-call-flags.patch...
> ERROR: spaces required around that '*' (ctx:WxV)
> #69: FILE: target/ppc/translate/vsx-impl.c.inc:1049:
> +bool trans_XSCVSPDPN(DisasContext *ctx, arg_XX2 *a)
>                                                   ^
> 
> My guess is that since the var 'arg_XX2' ends with a numeral the script
> thinks that the following '*' is an arithmetic operation. Problem is that
> we have other examples of this kind of declaration in the same file, e.g.:
> 
> 
> static bool trans_XVCVBF16SPN(DisasContext *ctx, arg_XX2 *a)
> 
> 
> 
> Is there a way to convince checkpatch.pl that this is an okay format?

Not that I know of.  I just ignore these parsing errors.


r~
Daniel Henrique Barboza May 23, 2022, 11:02 p.m. UTC | #4 
On 5/23/22 12:54, Richard Henderson wrote:
> On 5/23/22 06:48, Daniel Henrique Barboza wrote:
>> Checking 0006-target-ppc-declare-xscvspdpn-helper-with-call-flags.patch...
>> ERROR: spaces required around that '*' (ctx:WxV)
>> #69: FILE: target/ppc/translate/vsx-impl.c.inc:1049:
>> +bool trans_XSCVSPDPN(DisasContext *ctx, arg_XX2 *a)
>>                                                   ^
>>
>> My guess is that since the var 'arg_XX2' ends with a numeral the script
>> thinks that the following '*' is an arithmetic operation. Problem is that
>> we have other examples of this kind of declaration in the same file, e.g.:
>>
>>
>> static bool trans_XVCVBF16SPN(DisasContext *ctx, arg_XX2 *a)
>>
>>
>>
>> Is there a way to convince checkpatch.pl that this is an okay format?
> 
> Not that I know of.  I just ignore these parsing errors.
> 


Works for me. We should be aware that gitlab will complain about it when pushing
this to master though. E.g. https://gitlab.com/danielhb/qemu/-/jobs/2496047821 .

Thanks,


Daniel


> 
> r~
diff mbox series

Patch

diff --git a/target/ppc/fpu_helper.c b/target/ppc/fpu_helper.c
index b4d6f6ed4c..9bde333006 100644
--- a/target/ppc/fpu_helper.c
+++ b/target/ppc/fpu_helper.c
@@ -2875,11 +2875,6 @@  uint64_t helper_xscvdpspn(CPUPPCState *env, uint64_t xb)
     return (result << 32) | result;
 }
 
-uint64_t helper_xscvspdpn(CPUPPCState *env, uint64_t xb)
-{
-    return helper_todouble(xb >> 32);
-}
-
 /*
  * VSX_CVT_FP_TO_INT - VSX floating point to integer conversion
  *   op    - instruction mnemonic
diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 4a7cbdf922..5cee55176b 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -395,7 +395,6 @@  DEF_HELPER_3(XSCVSQQP, void, env, vsr, vsr)
 DEF_HELPER_3(xscvhpdp, void, env, vsr, vsr)
 DEF_HELPER_4(xscvsdqp, void, env, i32, vsr, vsr)
 DEF_HELPER_3(xscvspdp, void, env, vsr, vsr)
-DEF_HELPER_2(xscvspdpn, i64, env, i64)
 DEF_HELPER_3(xscvdpsxds, void, env, vsr, vsr)
 DEF_HELPER_3(xscvdpsxws, void, env, vsr, vsr)
 DEF_HELPER_3(xscvdpuxds, void, env, vsr, vsr)
diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index 1d0b55bde3..d4c2615b1a 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -708,6 +708,7 @@  XSCVUQQP        111111 ..... 00011 ..... 1101000100 -   @X_tb
 XSCVSQQP        111111 ..... 01011 ..... 1101000100 -   @X_tb
 XVCVBF16SPN     111100 ..... 10000 ..... 111011011 ..   @XX2
 XVCVSPBF16      111100 ..... 10001 ..... 111011011 ..   @XX2
+XSCVSPDPN       111100 ..... ----- ..... 101001011 ..   @XX2
 
 ## VSX Vector Test Least-Significant Bit by Byte Instruction
 
diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
index 3692740736..cc0601a14e 100644
--- a/target/ppc/translate/vsx-impl.c.inc
+++ b/target/ppc/translate/vsx-impl.c.inc
@@ -1045,7 +1045,31 @@  GEN_VSX_HELPER_R2(xscvqpuwz, 0x04, 0x1A, 0x01, PPC2_ISA300)
 GEN_VSX_HELPER_X2(xscvhpdp, 0x16, 0x15, 0x10, PPC2_ISA300)
 GEN_VSX_HELPER_R2(xscvsdqp, 0x04, 0x1A, 0x0A, PPC2_ISA300)
 GEN_VSX_HELPER_X2(xscvspdp, 0x12, 0x14, 0, PPC2_VSX)
-GEN_VSX_HELPER_XT_XB_ENV(xscvspdpn, 0x16, 0x14, 0, PPC2_VSX207)
+
+bool trans_XSCVSPDPN(DisasContext *ctx, arg_XX2 *a)
+{
+    TCGv_i64 t;
+    TCGv_i32 b;
+
+    REQUIRE_INSNS_FLAGS2(ctx, VSX207);
+    REQUIRE_VSX(ctx);
+
+    t = tcg_temp_new_i64();
+    b = tcg_temp_new_i32();
+
+    tcg_gen_ld_i32(b, cpu_env, offsetof(CPUPPCState, vsr[a->xb].VsrW(0)));
+
+    gen_helper_todouble(t, b);
+
+    set_cpu_vsr(a->xt, t, true);
+    set_cpu_vsr(a->xt, tcg_constant_i64(0), false);
+
+    tcg_temp_free_i64(t);
+    tcg_temp_free_i32(b);
+
+    return true;
+}
+
 GEN_VSX_HELPER_X2(xscvdpsxds, 0x10, 0x15, 0, PPC2_VSX)
 GEN_VSX_HELPER_X2(xscvdpsxws, 0x10, 0x05, 0, PPC2_VSX)
 GEN_VSX_HELPER_X2(xscvdpuxds, 0x10, 0x14, 0, PPC2_VSX)
diff --git a/target/ppc/translate/vsx-ops.c.inc b/target/ppc/translate/vsx-ops.c.inc
index b8fd116728..52d7ab30cd 100644
--- a/target/ppc/translate/vsx-ops.c.inc
+++ b/target/ppc/translate/vsx-ops.c.inc
@@ -200,7 +200,6 @@  GEN_XX2FORM(xscvdpspn, 0x16, 0x10, PPC2_VSX207),
 GEN_XX2FORM_EO(xscvhpdp, 0x16, 0x15, 0x10, PPC2_ISA300),
 GEN_VSX_XFORM_300_EO(xscvsdqp, 0x04, 0x1A, 0x0A, 0x00000001),
 GEN_XX2FORM(xscvspdp, 0x12, 0x14, PPC2_VSX),
-GEN_XX2FORM(xscvspdpn, 0x16, 0x14, PPC2_VSX207),
 GEN_XX2FORM(xscvdpsxds, 0x10, 0x15, PPC2_VSX),
 GEN_XX2FORM(xscvdpsxws, 0x10, 0x05, PPC2_VSX),
 GEN_XX2FORM(xscvdpuxds, 0x10, 0x14, PPC2_VSX),