| Message ID | 20240613093318.314913-3-rathc@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Move VSX storage access and compare insns to | expand |
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. 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~ >
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 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 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: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~ >
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~
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),
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(-)