diff mbox series

[2/4] target/ppc: Move VSX vector with length storage access insns to decodetree.

Message ID 20240607144921.726730-3-rathc@linux.ibm.com (mailing list archive)
State New
Headers show
Series target/ppc: Move VSX storage access and compare | expand

Commit Message

Chinmay Rath June 7, 2024, 2:49 p.m. UTC 
Moving the following instructions to decodetree specification :

        {l, st}xvl(l)           : X-form

The changes were verified by validating that the tcg-ops generated by those
instructions remain the same, which were captured using the '-d in_asm,op' flag.

Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
---
 target/ppc/helper.h                 |   8 +--
 target/ppc/insn32.decode            |   6 ++
 target/ppc/mem_helper.c             |   8 +--
 target/ppc/translate/vsx-impl.c.inc | 104 ++++++++++++++++++++--------
 target/ppc/translate/vsx-ops.c.inc  |   8 ---
 5 files changed, 89 insertions(+), 45 deletions(-)

Comments

Richard Henderson June 7, 2024, 3:41 p.m. UTC | #1 
On 6/7/24 07:49, Chinmay Rath wrote:
> +static bool do_ld_st_vl(DisasContext *ctx, arg_X *a,
> +                        void (*helper)(TCGv_ptr, TCGv, TCGv_ptr, TCGv))
> +{
> +    TCGv EA;
> +    TCGv_ptr xt;
> +    if (a->rt < 32) {
> +        REQUIRE_VSX(ctx);
> +    } else {
> +        REQUIRE_VECTOR(ctx);
> +    }
> +    xt = gen_vsr_ptr(a->rt);
> +    gen_set_access_type(ctx, ACCESS_INT);
> +
> +    if (a->ra) {
> +        EA = tcg_temp_new();
> +        tcg_gen_mov_tl(EA, cpu_gpr[a->ra]);
> +    } else {
> +        EA = tcg_constant_tl(0);
> +    }
> +    if (NARROW_MODE(ctx)) {
> +        tcg_gen_ext32u_tl(EA, EA);

ra == 0, narrow mode, will crash, due to write into constant 0.
Obviously 0 does not need extending, so this could be

     if (!a->ra) {
         ea = constant 0;
     } else if (narrow mode) {
         ea = tcg_temp_new();
         tcg_gen_ext32u_tl(ea, cpu_gpr[a->ra]);
     } else {
         ra = cpu_gpr[a->ra];
     }


Aren't there existing helper functions for computing this address?
And if not, better to create one.


r~
Chinmay Rath June 9, 2024, 6:11 p.m. UTC | #2 
Hi Richard,

On 6/7/24 21:11, Richard Henderson wrote:
> On 6/7/24 07:49, Chinmay Rath wrote:
>> +static bool do_ld_st_vl(DisasContext *ctx, arg_X *a,
>> +                        void (*helper)(TCGv_ptr, TCGv, TCGv_ptr, TCGv))
>> +{
>> +    TCGv EA;
>> +    TCGv_ptr xt;
>> +    if (a->rt < 32) {
>> +        REQUIRE_VSX(ctx);
>> +    } else {
>> +        REQUIRE_VECTOR(ctx);
>> +    }
>> +    xt = gen_vsr_ptr(a->rt);
>> +    gen_set_access_type(ctx, ACCESS_INT);
>> +
>> +    if (a->ra) {
>> +        EA = tcg_temp_new();
>> +        tcg_gen_mov_tl(EA, cpu_gpr[a->ra]);
>> +    } else {
>> +        EA = tcg_constant_tl(0);
>> +    }
>> +    if (NARROW_MODE(ctx)) {
>> +        tcg_gen_ext32u_tl(EA, EA);
>
> ra == 0, narrow mode, will crash, due to write into constant 0.
> Obviously 0 does not need extending, so this could be
>
>     if (!a->ra) {
>         ea = constant 0;
>     } else if (narrow mode) {
>         ea = tcg_temp_new();
>         tcg_gen_ext32u_tl(ea, cpu_gpr[a->ra]);
>     } else {
>         ra = cpu_gpr[a->ra];
>     }
>
^ Thank you Richard, will take care in v2.
>
> Aren't there existing helper functions for computing this address?
> And if not, better to create one.
^
The calculation of effective address in these instructions is slightly 
different than the others,
for which helper function exist :

EA for these insns : EA ← (RA=0) ? 0 : GPR[RA]
EA for rest storage access insns : EA ← ((RA=0) ? 0 : GPR[RA]) + GPR[RB]

This is why I could not reuse that function. Also, this calculation of 
EA is limited to these
4 insns above, and only 2 others (prefixed insns), which is why I did 
not create a new function
for this, considering it won't be reused for any other insn.

Please let me know if I should create a new function in this case as well.

Thanks and Regards,
Chinmay
>
>
> r~
>
Richard Henderson June 9, 2024, 6:20 p.m. UTC | #3 
On 6/9/24 11:11, Chinmay Rath wrote:
> The calculation of effective address in these instructions is slightly different than the 
> others,
> for which helper function exist :
> 
> EA for these insns : EA ← (RA=0) ? 0 : GPR[RA]
> EA for rest storage access insns : EA ← ((RA=0) ? 0 : GPR[RA]) + GPR[RB]
> 
> This is why I could not reuse that function. Also, this calculation of EA is limited to these
> 4 insns above, and only 2 others (prefixed insns), which is why I did not create a new 
> function
> for this, considering it won't be reused for any other insn.
> 
> Please let me know if I should create a new function in this case as well.

If you expect this to be used just once, then leaving it inline is perfectly fine.


r~
diff mbox series

Patch

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 3b4a0c4674..510ce76524 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -274,10 +274,10 @@  DEF_HELPER_3(stvebx, void, env, avr, tl)
 DEF_HELPER_3(stvehx, void, env, avr, tl)
 DEF_HELPER_3(stvewx, void, env, avr, tl)
 #if defined(TARGET_PPC64)
-DEF_HELPER_4(lxvl, void, env, tl, vsr, tl)
-DEF_HELPER_4(lxvll, void, env, tl, vsr, tl)
-DEF_HELPER_4(stxvl, void, env, tl, vsr, tl)
-DEF_HELPER_4(stxvll, void, env, tl, vsr, tl)
+DEF_HELPER_4(LXVL, void, env, tl, vsr, tl)
+DEF_HELPER_4(LXVLL, void, env, tl, vsr, tl)
+DEF_HELPER_4(STXVL, void, env, tl, vsr, tl)
+DEF_HELPER_4(STXVLL, void, env, tl, vsr, tl)
 #endif
 DEF_HELPER_4(vsumsws, void, env, avr, avr, avr)
 DEF_HELPER_4(vsum2sws, void, env, avr, avr, avr)
diff --git a/target/ppc/insn32.decode b/target/ppc/insn32.decode
index 88753c75e1..445fdb341f 100644
--- a/target/ppc/insn32.decode
+++ b/target/ppc/insn32.decode
@@ -805,6 +805,12 @@  STXSIHX         011111 ..... ..... ..... 1110101101 .   @X_TSX
 STXSIWX         011111 ..... ..... ..... 0010001100 .   @X_TSX
 STXSSPX         011111 ..... ..... ..... 1010001100 .   @X_TSX
 
+LXVL            011111 ..... ..... ..... 0100001101 .   @X_TSX
+LXVLL           011111 ..... ..... ..... 0100101101 .   @X_TSX
+
+STXVL           011111 ..... ..... ..... 0110001101 .   @X_TSX
+STXVLL          011111 ..... ..... ..... 0110101101 .   @X_TSX
+
 ## VSX Vector Binary Floating-Point Sign Manipulation Instructions
 
 XVABSDP         111100 ..... 00000 ..... 111011001 ..   @XX2
diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
index ea7e8443a8..dec1b25eb8 100644
--- a/target/ppc/mem_helper.c
+++ b/target/ppc/mem_helper.c
@@ -467,8 +467,8 @@  void helper_##name(CPUPPCState *env, target_ulong addr,                 \
     *xt = t;                                                            \
 }
 
-VSX_LXVL(lxvl, 0)
-VSX_LXVL(lxvll, 1)
+VSX_LXVL(LXVL, 0)
+VSX_LXVL(LXVLL, 1)
 #undef VSX_LXVL
 
 #define VSX_STXVL(name, lj)                                       \
@@ -496,8 +496,8 @@  void helper_##name(CPUPPCState *env, target_ulong addr,           \
     }                                                             \
 }
 
-VSX_STXVL(stxvl, 0)
-VSX_STXVL(stxvll, 1)
+VSX_STXVL(STXVL, 0)
+VSX_STXVL(STXVLL, 1)
 #undef VSX_STXVL
 #undef GET_NB
 #endif /* TARGET_PPC64 */
diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
index de2a26a213..695b75ded9 100644
--- a/target/ppc/translate/vsx-impl.c.inc
+++ b/target/ppc/translate/vsx-impl.c.inc
@@ -232,36 +232,82 @@  static void gen_lxvb16x(DisasContext *ctx)
     set_cpu_vsr(xT(ctx->opcode), xtl, false);
 }
 
-#ifdef TARGET_PPC64
-#define VSX_VECTOR_LOAD_STORE_LENGTH(name)                         \
-static void gen_##name(DisasContext *ctx)                          \
-{                                                                  \
-    TCGv EA;                                                       \
-    TCGv_ptr xt;                                                   \
-                                                                   \
-    if (xT(ctx->opcode) < 32) {                                    \
-        if (unlikely(!ctx->vsx_enabled)) {                         \
-            gen_exception(ctx, POWERPC_EXCP_VSXU);                 \
-            return;                                                \
-        }                                                          \
-    } else {                                                       \
-        if (unlikely(!ctx->altivec_enabled)) {                     \
-            gen_exception(ctx, POWERPC_EXCP_VPU);                  \
-            return;                                                \
-        }                                                          \
-    }                                                              \
-    EA = tcg_temp_new();                                           \
-    xt = gen_vsr_ptr(xT(ctx->opcode));                             \
-    gen_set_access_type(ctx, ACCESS_INT);                          \
-    gen_addr_register(ctx, EA);                                    \
-    gen_helper_##name(tcg_env, EA, xt, cpu_gpr[rB(ctx->opcode)]);  \
-}
-
-VSX_VECTOR_LOAD_STORE_LENGTH(lxvl)
-VSX_VECTOR_LOAD_STORE_LENGTH(lxvll)
-VSX_VECTOR_LOAD_STORE_LENGTH(stxvl)
-VSX_VECTOR_LOAD_STORE_LENGTH(stxvll)
+#if defined(TARGET_PPC64)
+static bool do_ld_st_vl(DisasContext *ctx, arg_X *a,
+                        void (*helper)(TCGv_ptr, TCGv, TCGv_ptr, TCGv))
+{
+    TCGv EA;
+    TCGv_ptr xt;
+    if (a->rt < 32) {
+        REQUIRE_VSX(ctx);
+    } else {
+        REQUIRE_VECTOR(ctx);
+    }
+    xt = gen_vsr_ptr(a->rt);
+    gen_set_access_type(ctx, ACCESS_INT);
+
+    if (a->ra) {
+        EA = tcg_temp_new();
+        tcg_gen_mov_tl(EA, cpu_gpr[a->ra]);
+    } else {
+        EA = tcg_constant_tl(0);
+    }
+    if (NARROW_MODE(ctx)) {
+        tcg_gen_ext32u_tl(EA, EA);
+    }
+
+    helper(tcg_env, EA, xt, cpu_gpr[a->rb]);
+    return true;
+}
+#endif
+
+static bool trans_LXVL(DisasContext *ctx, arg_LXVL *a)
+{
+    REQUIRE_64BIT(ctx);
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+#if defined(TARGET_PPC64)
+    return do_ld_st_vl(ctx, a, gen_helper_LXVL);
+#else
+    qemu_build_not_reached();
+#endif
+    return true;
+}
+
+static bool trans_LXVLL(DisasContext *ctx, arg_LXVLL *a)
+{
+    REQUIRE_64BIT(ctx);
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+#if defined(TARGET_PPC64)
+    return do_ld_st_vl(ctx, a, gen_helper_LXVLL);
+#else
+    qemu_build_not_reached();
+#endif
+    return true;
+}
+
+static bool trans_STXVL(DisasContext *ctx, arg_STXVL *a)
+{
+    REQUIRE_64BIT(ctx);
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+#if defined(TARGET_PPC64)
+    return do_ld_st_vl(ctx, a, gen_helper_STXVL);
+#else
+    qemu_build_not_reached();
 #endif
+    return true;
+}
+
+static bool trans_STXVLL(DisasContext *ctx, arg_STXVLL *a)
+{
+    REQUIRE_64BIT(ctx);
+    REQUIRE_INSNS_FLAGS2(ctx, ISA300);
+#if defined(TARGET_PPC64)
+    return do_ld_st_vl(ctx, a, gen_helper_STXVLL);
+#else
+    qemu_build_not_reached();
+#endif
+    return true;
+}
 
 static bool do_stxs(DisasContext *ctx, arg_X *a,
                     void (*op)(DisasContext *, TCGv_i64, TCGv))
diff --git a/target/ppc/translate/vsx-ops.c.inc b/target/ppc/translate/vsx-ops.c.inc
index d44cb55836..7f4326c974 100644
--- a/target/ppc/translate/vsx-ops.c.inc
+++ b/target/ppc/translate/vsx-ops.c.inc
@@ -4,19 +4,11 @@  GEN_HANDLER_E(lxvdsx, 0x1F, 0x0C, 0x0A, 0, PPC_NONE, PPC2_VSX),
 GEN_HANDLER_E(lxvw4x, 0x1F, 0x0C, 0x18, 0, PPC_NONE, PPC2_VSX),
 GEN_HANDLER_E(lxvh8x, 0x1F, 0x0C, 0x19, 0, PPC_NONE,  PPC2_ISA300),
 GEN_HANDLER_E(lxvb16x, 0x1F, 0x0C, 0x1B, 0, PPC_NONE, PPC2_ISA300),
-#if defined(TARGET_PPC64)
-GEN_HANDLER_E(lxvl, 0x1F, 0x0D, 0x08, 0, PPC_NONE, PPC2_ISA300),
-GEN_HANDLER_E(lxvll, 0x1F, 0x0D, 0x09, 0, PPC_NONE, PPC2_ISA300),
-#endif
 
 GEN_HANDLER_E(stxvd2x, 0x1F, 0xC, 0x1E, 0, PPC_NONE, PPC2_VSX),
 GEN_HANDLER_E(stxvw4x, 0x1F, 0xC, 0x1C, 0, PPC_NONE, PPC2_VSX),
 GEN_HANDLER_E(stxvh8x, 0x1F, 0x0C, 0x1D, 0, PPC_NONE,  PPC2_ISA300),
 GEN_HANDLER_E(stxvb16x, 0x1F, 0x0C, 0x1F, 0, PPC_NONE, PPC2_ISA300),
-#if defined(TARGET_PPC64)
-GEN_HANDLER_E(stxvl, 0x1F, 0x0D, 0x0C, 0, PPC_NONE, PPC2_ISA300),
-GEN_HANDLER_E(stxvll, 0x1F, 0x0D, 0x0D, 0, PPC_NONE, PPC2_ISA300),
-#endif
 
 GEN_HANDLER_E(mfvsrwz, 0x1F, 0x13, 0x03, 0x0000F800, PPC_NONE, PPC2_VSX207),
 GEN_HANDLER_E(mtvsrwa, 0x1F, 0x13, 0x06, 0x0000F800, PPC_NONE, PPC2_VSX207),