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.
Also added a new function to calculate the effective address :
EA <- (RA == 0) ? 0 : GPR[RA], which is now used by the above-said insns,
and shall be used later by (p){lx, stx}vp insns.
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.c | 18 ++++++
target/ppc/translate/vsx-impl.c.inc | 94 ++++++++++++++++++++---------
target/ppc/translate/vsx-ops.c.inc | 8 ---
6 files changed, 97 insertions(+), 45 deletions(-)
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.c b/target/ppc/translate.c
index 93ffec787c..a1f2f4fbda 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3096,6 +3096,7 @@ static inline void gen_align_no_le(DisasContext *ctx)
(ctx->opcode & 0x03FF0000) | POWERPC_EXCP_ALIGN_LE);
}
+/* EA <- {(ra == 0) ? 0 : GPR[ra]} + displ */
static TCGv do_ea_calc(DisasContext *ctx, int ra, TCGv displ)
{
TCGv ea = tcg_temp_new();
@@ -3110,6 +3111,23 @@ static TCGv do_ea_calc(DisasContext *ctx, int ra, TCGv displ)
return ea;
}
+/* EA <- (ra == 0) ? 0 : GPR[ra] */
+static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
+{
+ TCGv EA;
+ if (!ra) {
+ EA = tcg_constant_tl(0);
+ return EA;
+ }
+ EA = tcg_temp_new();
+ if (NARROW_MODE(ctx)) {
+ tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
+ } else {
+ tcg_gen_mov_tl(EA, cpu_gpr[ra]);
+ }
+ return EA;
+}
+
/*** Integer load ***/
#define DEF_MEMOP(op) ((op) | ctx->default_tcg_memop_mask)
#define BSWAP_MEMOP(op) ((op) | (ctx->default_tcg_memop_mask ^ MO_BSWAP))
diff --git a/target/ppc/translate/vsx-impl.c.inc b/target/ppc/translate/vsx-impl.c.inc
index de2a26a213..46bab49215 100644
--- a/target/ppc/translate/vsx-impl.c.inc
+++ b/target/ppc/translate/vsx-impl.c.inc
@@ -232,36 +232,72 @@ 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);
+ EA = do_ea_calc_ra(ctx, a->ra);
+ 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),
--
2.39.3On 6/13/24 02:33, Chinmay Rath wrote:
> +/* EA <- (ra == 0) ? 0 : GPR[ra] */
> +static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
> +{
> + TCGv EA;
> + if (!ra) {
> + EA = tcg_constant_tl(0);
> + return EA;
> + }
> + EA = tcg_temp_new();
> + if (NARROW_MODE(ctx)) {
> + tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
> + } else {
> + tcg_gen_mov_tl(EA, cpu_gpr[ra]);
Why are you making a copy, rather than just returning cpu_gpr[ra]?
If you need to modify the resulting EA, then you also need to make a copy for 0.
r~
Hi Richard,
On 6/17/24 00:43, Richard Henderson wrote:
> On 6/13/24 02:33, Chinmay Rath wrote:
>> +/* EA <- (ra == 0) ? 0 : GPR[ra] */
>> +static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
>> +{
>> + TCGv EA;
>> + if (!ra) {
>> + EA = tcg_constant_tl(0);
>> + return EA;
>> + }
>> + EA = tcg_temp_new();
>> + if (NARROW_MODE(ctx)) {
>> + tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
>> + } else {
>> + tcg_gen_mov_tl(EA, cpu_gpr[ra]);
>
> Why are you making a copy, rather than just returning cpu_gpr[ra]?
> If you need to modify the resulting EA, then you also need to make a
> copy for 0.
>
Please ignore my previous response.
I think do_ea_calc_ra should allow modification to the resulting EA,
hence below change appears more appropriate to me :
/* EA <- (ra == 0) ? 0 : GPR[ra] */
static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
{
TCGv EA = tcg_temp_new();
if (!ra) {
tcg_gen_movi_tl(EA, 0);
return EA;
}
if (NARROW_MODE(ctx)) {
tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
} else {
tcg_gen_mov_tl(EA, cpu_gpr[ra]);
}
return EA;
}
Let me know your thoughts.
Thanks & Regards,
Chinmay
>
> r~
>
On 6/17/24 04:51, Chinmay Rath wrote:
>
>
> Hi Richard,
> On 6/17/24 00:43, Richard Henderson wrote:
>> On 6/13/24 02:33, Chinmay Rath wrote:
>>> +/* EA <- (ra == 0) ? 0 : GPR[ra] */
>>> +static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
>>> +{
>>> + TCGv EA;
>>> + if (!ra) {
>>> + EA = tcg_constant_tl(0);
>>> + return EA;
>>> + }
>>> + EA = tcg_temp_new();
>>> + if (NARROW_MODE(ctx)) {
>>> + tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
>>> + } else {
>>> + tcg_gen_mov_tl(EA, cpu_gpr[ra]);
>>
>> Why are you making a copy, rather than just returning cpu_gpr[ra]?
>> If you need to modify the resulting EA, then you also need to make a copy for 0.
>>
> Please ignore my previous response.
> I think do_ea_calc_ra should allow modification to the resulting EA, hence below change
> appears more appropriate to me :
>
> /* EA <- (ra == 0) ? 0 : GPR[ra] */
> static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
> {
> TCGv EA = tcg_temp_new();
> if (!ra) {
> tcg_gen_movi_tl(EA, 0);
> return EA;
> }
> if (NARROW_MODE(ctx)) {
> tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
> } else {
> tcg_gen_mov_tl(EA, cpu_gpr[ra]);
> }
> return EA;
> }
If that's what's needed by the callers of do_ea_calc_ra, then yes.
You can drop the first return EA and use else if instead.
r~
On 6/17/24 23:27, Richard Henderson wrote:
> On 6/17/24 04:51, Chinmay Rath wrote:
>>
>>
>> Hi Richard,
>> On 6/17/24 00:43, Richard Henderson wrote:
>>> On 6/13/24 02:33, Chinmay Rath wrote:
>>>> +/* EA <- (ra == 0) ? 0 : GPR[ra] */
>>>> +static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
>>>> +{
>>>> + TCGv EA;
>>>> + if (!ra) {
>>>> + EA = tcg_constant_tl(0);
>>>> + return EA;
>>>> + }
>>>> + EA = tcg_temp_new();
>>>> + if (NARROW_MODE(ctx)) {
>>>> + tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
>>>> + } else {
>>>> + tcg_gen_mov_tl(EA, cpu_gpr[ra]);
>>>
>>> Why are you making a copy, rather than just returning cpu_gpr[ra]?
>>> If you need to modify the resulting EA, then you also need to make a
>>> copy for 0.
>>>
>> Please ignore my previous response.
>> I think do_ea_calc_ra should allow modification to the resulting EA,
>> hence below change appears more appropriate to me :
>>
>> /* EA <- (ra == 0) ? 0 : GPR[ra] */
>> static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
>> {
>> TCGv EA = tcg_temp_new();
>> if (!ra) {
>> tcg_gen_movi_tl(EA, 0);
>> return EA;
>> }
>> if (NARROW_MODE(ctx)) {
>> tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
>> } else {
>> tcg_gen_mov_tl(EA, cpu_gpr[ra]);
>> }
>> return EA;
>> }
>
> If that's what's needed by the callers of do_ea_calc_ra, then yes.
> You can drop the first return EA and use else if instead.
Sure.
I shall stick to keeping EA modifiable, (even though it is not modified
by the callers in this patch),
to allow its proper usage by (p){lx, stx}vp insns in future.
Thanks & Regards,
Chinmay
>
>
> r~
Hi Richard,
On 6/17/24 00:43, Richard Henderson wrote:
> On 6/13/24 02:33, Chinmay Rath wrote:
>> +/* EA <- (ra == 0) ? 0 : GPR[ra] */
>> +static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
>> +{
>> + TCGv EA;
>> + if (!ra) {
>> + EA = tcg_constant_tl(0);
>> + return EA;
>> + }
>> + EA = tcg_temp_new();
>> + if (NARROW_MODE(ctx)) {
>> + tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
>> + } else {
>> + tcg_gen_mov_tl(EA, cpu_gpr[ra]);
>
> Why are you making a copy, rather than just returning cpu_gpr[ra]?
True, this tcg move is redundant. Was carried away to maintain
uniformity with the original do_ea_calc function. My bad!
This can rather just be :
/* ea <- (ra == 0) ? 0 : GPR[ra] */
static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
{
TCGv EA;
if (!ra) {
return tcg_constant_tl(0);
}
if (NARROW_MODE(ctx)) {
EA = tcg_temp_new();
tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
} else {
return cpu_gpr[ra];
}
return EA;
}
> If you need to modify the resulting EA, then you also need to make a
> copy for 0.
>
Hey, didn't properly get what you meant here.
Did you mean : Since I'm using a tcg_constant for 0, if the EA is to be
modified later, this constant would be an issue, in which case, I should
make a copy for it ??
Considering that, there are no tcg level modifications with this EA.
However, the underlying helper method, which considers this EA as a
target_ulong type does modify it, which I don't think should be an issue.
Please let me know if I missed something.
Thanks & Regards,
Chinmay
> r~
>
On 6/17/24 03:40, Chinmay Rath wrote:
> static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
> {
> TCGv EA;
> if (!ra) {
> return tcg_constant_tl(0);
> }
> if (NARROW_MODE(ctx)) {
> EA = tcg_temp_new();
> tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
> } else {
> return cpu_gpr[ra];
> }
> return EA;
> }
>
>> If you need to modify the resulting EA, then you also need to make a copy for 0.
>>
> Hey, didn't properly get what you meant here.
> Did you mean : Since I'm using a tcg_constant for 0, if the EA is to be modified later,
> this constant would be an issue, in which case, I should make a copy for it ??
Yes.
> Considering that, there are no tcg level modifications with this EA.
Ok, good.
> However, the
> underlying helper method, which considers this EA as a target_ulong type does modify it,
> which I don't think should be an issue.
Correct, that's fine.
r~
On 6/17/24 23:15, Richard Henderson wrote:
> On 6/17/24 03:40, Chinmay Rath wrote:
>> static TCGv do_ea_calc_ra(DisasContext *ctx, int ra)
>> {
>> TCGv EA;
>> if (!ra) {
>> return tcg_constant_tl(0);
>> }
>> if (NARROW_MODE(ctx)) {
>> EA = tcg_temp_new();
>> tcg_gen_ext32u_tl(EA, cpu_gpr[ra]);
>> } else {
>> return cpu_gpr[ra];
>> }
>> return EA;
>> }
>>
>>> If you need to modify the resulting EA, then you also need to make a
>>> copy for 0.
>>>
>> Hey, didn't properly get what you meant here.
>> Did you mean : Since I'm using a tcg_constant for 0, if the EA is to
>> be modified later, this constant would be an issue, in which case, I
>> should make a copy for it ??
>
> Yes.
>
>> Considering that, there are no tcg level modifications with this EA.
>
> Ok, good.
>
>
>> However, the underlying helper method, which considers this EA as a
>> target_ulong type does modify it, which I don't think should be an
>> issue.
>
> Correct, that's fine.
Awesome ! Thanks for the clarification.
Regards,
Chinmay
>
>
> r~
>
© 2016 - 2024 Red Hat, Inc.