| Message ID | 20190428143845.11810-3-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | target/ppc: remove getVSR()/putVSR() and further tidy-up | expand |
On 4/28/19 7:38 AM, Mark Cave-Ayland wrote: > #define VSX_LXVL(name, lj) \ > void helper_##name(CPUPPCState *env, target_ulong addr, \ > - target_ulong xt_num, target_ulong rb) \ > + target_ulong xt, target_ulong rb) \ > { \ > + ppc_vsr_t *r = &env->vsr[xt]; \ > + int nb = GET_NB(env->gpr[rb]); \ > int i; \ > - ppc_vsr_t xt; \ > - uint64_t nb = GET_NB(rb); \ > \ > - xt.s128 = int128_zero(); \ > + r->s128 = int128_zero(); \ > if (nb) { \ > nb = (nb >= 16) ? 16 : nb; \ > if (msr_le && !lj) { \ > for (i = 16; i > 16 - nb; i--) { \ > - xt.VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC()); \ > + r->VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC()); \ > addr = addr_add(env, addr, 1); \ > } \ > } else { \ > for (i = 0; i < nb; i++) { \ > - xt.VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC()); \ > + r->VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC()); \ > addr = addr_add(env, addr, 1); \ > } \ > } \ > } \ > - putVSR(xt_num, &xt, env); \ > } Similarly, this modifies env->vsr[xt] before all exceptions are recognized. > @@ -304,12 +304,14 @@ static void gen_##name(DisasContext *ctx) \ > } \ > } \ > EA = tcg_temp_new(); \ > - xt = tcg_const_tl(xT(ctx->opcode)); \ > gen_set_access_type(ctx, ACCESS_INT); \ > gen_addr_register(ctx, EA); \ > - gen_helper_##name(cpu_env, EA, xt, cpu_gpr[rB(ctx->opcode)]); \ > + xt = tcg_const_tl(xT(ctx->opcode)); \ > + rb = tcg_const_tl(rB(ctx->opcode)); \ > + gen_helper_##name(cpu_env, EA, xt, rb); \ > tcg_temp_free(EA); \ > tcg_temp_free(xt); \ > + tcg_temp_free(rb); \ > } Why are you adjusting the function to pass the rB register number rather than the contents of rB? That seems the wrong way around... r~
On 30/04/2019 17:29, Richard Henderson wrote: > On 4/28/19 7:38 AM, Mark Cave-Ayland wrote: >> #define VSX_LXVL(name, lj) \ >> void helper_##name(CPUPPCState *env, target_ulong addr, \ >> - target_ulong xt_num, target_ulong rb) \ >> + target_ulong xt, target_ulong rb) \ >> { \ >> + ppc_vsr_t *r = &env->vsr[xt]; \ >> + int nb = GET_NB(env->gpr[rb]); \ >> int i; \ >> - ppc_vsr_t xt; \ >> - uint64_t nb = GET_NB(rb); \ >> \ >> - xt.s128 = int128_zero(); \ >> + r->s128 = int128_zero(); \ >> if (nb) { \ >> nb = (nb >= 16) ? 16 : nb; \ >> if (msr_le && !lj) { \ >> for (i = 16; i > 16 - nb; i--) { \ >> - xt.VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC()); \ >> + r->VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC()); \ >> addr = addr_add(env, addr, 1); \ >> } \ >> } else { \ >> for (i = 0; i < nb; i++) { \ >> - xt.VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC()); \ >> + r->VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC()); \ >> addr = addr_add(env, addr, 1); \ >> } \ >> } \ >> } \ >> - putVSR(xt_num, &xt, env); \ >> } > > Similarly, this modifies env->vsr[xt] before all exceptions are recognized. Okay - if you're happy with my previous suggestion with the VSR* macros and the local variable then I can make the same change here too. >> @@ -304,12 +304,14 @@ static void gen_##name(DisasContext *ctx) \ >> } \ >> } \ >> EA = tcg_temp_new(); \ >> - xt = tcg_const_tl(xT(ctx->opcode)); \ >> gen_set_access_type(ctx, ACCESS_INT); \ >> gen_addr_register(ctx, EA); \ >> - gen_helper_##name(cpu_env, EA, xt, cpu_gpr[rB(ctx->opcode)]); \ >> + xt = tcg_const_tl(xT(ctx->opcode)); \ >> + rb = tcg_const_tl(rB(ctx->opcode)); \ >> + gen_helper_##name(cpu_env, EA, xt, rb); \ >> tcg_temp_free(EA); \ >> tcg_temp_free(xt); \ >> + tcg_temp_free(rb); \ >> } > > Why are you adjusting the function to pass the rB register number rather than > the contents of rB? That seems the wrong way around... I think what I was trying to do here was eliminate the cpu_gpr since it feels to me that with the vector patchsets and your negative offset patches that this should be the way to go for accessing CPUState rather than using TCG globals. Looking at this again I realise the solution is really the same as is currently used for gen_load_spr() so I can use something like this: static inline void gen_load_gpr(TCGv t, int reg) { tcg_gen_ld_tl(t, cpu_env, offsetof(CPUPPCState, gpr[reg])); } Does this seem reasonable as a solution? ATB, Mark.
On 5/5/19 2:34 AM, Mark Cave-Ayland wrote: >>> EA = tcg_temp_new(); \ >>> - xt = tcg_const_tl(xT(ctx->opcode)); \ >>> gen_set_access_type(ctx, ACCESS_INT); \ >>> gen_addr_register(ctx, EA); \ >>> - gen_helper_##name(cpu_env, EA, xt, cpu_gpr[rB(ctx->opcode)]); \ >>> + xt = tcg_const_tl(xT(ctx->opcode)); \ >>> + rb = tcg_const_tl(rB(ctx->opcode)); \ >>> + gen_helper_##name(cpu_env, EA, xt, rb); \ >>> tcg_temp_free(EA); \ >>> tcg_temp_free(xt); \ >>> + tcg_temp_free(rb); \ >>> } >> >> Why are you adjusting the function to pass the rB register number rather than >> the contents of rB? That seems the wrong way around... > > I think what I was trying to do here was eliminate the cpu_gpr since it > feels to me that with the vector patchsets and your negative offset patches > that this should be the way to go for accessing CPUState rather than using > TCG globals. Not for the integer register set. > Looking at this again I realise the solution is really the same as is > currently used > for gen_load_spr() so I can use something like this:> > static inline void gen_load_gpr(TCGv t, int reg) > { > tcg_gen_ld_tl(t, cpu_env, offsetof(CPUPPCState, gpr[reg])); > } > > Does this seem reasonable as a solution? No, this will fail quickly. r~
On 05/05/2019 15:34, Richard Henderson wrote: > On 5/5/19 2:34 AM, Mark Cave-Ayland wrote: >>>> EA = tcg_temp_new(); \ >>>> - xt = tcg_const_tl(xT(ctx->opcode)); \ >>>> gen_set_access_type(ctx, ACCESS_INT); \ >>>> gen_addr_register(ctx, EA); \ >>>> - gen_helper_##name(cpu_env, EA, xt, cpu_gpr[rB(ctx->opcode)]); \ >>>> + xt = tcg_const_tl(xT(ctx->opcode)); \ >>>> + rb = tcg_const_tl(rB(ctx->opcode)); \ >>>> + gen_helper_##name(cpu_env, EA, xt, rb); \ >>>> tcg_temp_free(EA); \ >>>> tcg_temp_free(xt); \ >>>> + tcg_temp_free(rb); \ >>>> } >>> >>> Why are you adjusting the function to pass the rB register number rather than >>> the contents of rB? That seems the wrong way around... >> >> I think what I was trying to do here was eliminate the cpu_gpr since it >> feels to me that with the vector patchsets and your negative offset patches >> that this should be the way to go for accessing CPUState rather than using >> TCG globals. > > Not for the integer register set. > >> Looking at this again I realise the solution is really the same as is >> currently used >> for gen_load_spr() so I can use something like this:> >> static inline void gen_load_gpr(TCGv t, int reg) >> { >> tcg_gen_ld_tl(t, cpu_env, offsetof(CPUPPCState, gpr[reg])); >> } >> >> Does this seem reasonable as a solution? > > No, this will fail quickly. Okay in that case I'll leave it as-is. So just to satisfy my curiosity here: is the problem here the mixing and matching of offsets and TCG globals, rather than the use of offsets as done for the VMX/VSX registers? ATB, Mark.
On 5/5/19 8:49 AM, Mark Cave-Ayland wrote: > Okay in that case I'll leave it as-is. So just to satisfy my curiosity here: is the > problem here the mixing and matching of offsets and TCG globals, rather than the use > of offsets as done for the VMX/VSX registers? Correct. r~
diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c index 5b0f9ee50d..4dfa7ee23f 100644 --- a/target/ppc/mem_helper.c +++ b/target/ppc/mem_helper.c @@ -415,28 +415,27 @@ STVE(stvewx, cpu_stl_data_ra, bswap32, u32) #define VSX_LXVL(name, lj) \ void helper_##name(CPUPPCState *env, target_ulong addr, \ - target_ulong xt_num, target_ulong rb) \ + target_ulong xt, target_ulong rb) \ { \ + ppc_vsr_t *r = &env->vsr[xt]; \ + int nb = GET_NB(env->gpr[rb]); \ int i; \ - ppc_vsr_t xt; \ - uint64_t nb = GET_NB(rb); \ \ - xt.s128 = int128_zero(); \ + r->s128 = int128_zero(); \ if (nb) { \ nb = (nb >= 16) ? 16 : nb; \ if (msr_le && !lj) { \ for (i = 16; i > 16 - nb; i--) { \ - xt.VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC()); \ + r->VsrB(i - 1) = cpu_ldub_data_ra(env, addr, GETPC()); \ addr = addr_add(env, addr, 1); \ } \ } else { \ for (i = 0; i < nb; i++) { \ - xt.VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC()); \ + r->VsrB(i) = cpu_ldub_data_ra(env, addr, GETPC()); \ addr = addr_add(env, addr, 1); \ } \ } \ } \ - putVSR(xt_num, &xt, env); \ } VSX_LXVL(lxvl, 0) @@ -445,25 +444,24 @@ VSX_LXVL(lxvll, 1) #define VSX_STXVL(name, lj) \ void helper_##name(CPUPPCState *env, target_ulong addr, \ - target_ulong xt_num, target_ulong rb) \ + target_ulong xt, target_ulong rb) \ { \ + ppc_vsr_t *r = &env->vsr[xt]; \ + int nb = GET_NB(env->gpr[rb]); \ int i; \ - ppc_vsr_t xt; \ - target_ulong nb = GET_NB(rb); \ \ if (!nb) { \ return; \ } \ - getVSR(xt_num, &xt, env); \ nb = (nb >= 16) ? 16 : nb; \ if (msr_le && !lj) { \ for (i = 16; i > 16 - nb; i--) { \ - cpu_stb_data_ra(env, addr, xt.VsrB(i - 1), GETPC()); \ + cpu_stb_data_ra(env, addr, r->VsrB(i - 1), GETPC()); \ addr = addr_add(env, addr, 1); \ } \ } else { \ for (i = 0; i < nb; i++) { \ - cpu_stb_data_ra(env, addr, xt.VsrB(i), GETPC()); \ + cpu_stb_data_ra(env, addr, r->VsrB(i), GETPC()); \ addr = addr_add(env, addr, 1); \ } \ } \ diff --git a/target/ppc/translate/vsx-impl.inc.c b/target/ppc/translate/vsx-impl.inc.c index 11d9b75d01..c776d50ddc 100644 --- a/target/ppc/translate/vsx-impl.inc.c +++ b/target/ppc/translate/vsx-impl.inc.c @@ -290,7 +290,7 @@ VSX_VECTOR_LOAD_STORE(stxvx, st_i64, 1) #define VSX_VECTOR_LOAD_STORE_LENGTH(name) \ static void gen_##name(DisasContext *ctx) \ { \ - TCGv EA, xt; \ + TCGv EA, xt, rb; \ \ if (xT(ctx->opcode) < 32) { \ if (unlikely(!ctx->vsx_enabled)) { \ @@ -304,12 +304,14 @@ static void gen_##name(DisasContext *ctx) \ } \ } \ EA = tcg_temp_new(); \ - xt = tcg_const_tl(xT(ctx->opcode)); \ gen_set_access_type(ctx, ACCESS_INT); \ gen_addr_register(ctx, EA); \ - gen_helper_##name(cpu_env, EA, xt, cpu_gpr[rB(ctx->opcode)]); \ + xt = tcg_const_tl(xT(ctx->opcode)); \ + rb = tcg_const_tl(rB(ctx->opcode)); \ + gen_helper_##name(cpu_env, EA, xt, rb); \ tcg_temp_free(EA); \ tcg_temp_free(xt); \ + tcg_temp_free(rb); \ } VSX_VECTOR_LOAD_STORE_LENGTH(lxvl)
Since commit 8a14d31b00 "target/ppc: switch fpr/vsrl registers so all VSX registers are in host endian order" functions getVSR() and putVSR() which used to convert the VSR registers into host endian order are no longer required. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- target/ppc/mem_helper.c | 24 +++++++++++------------- target/ppc/translate/vsx-impl.inc.c | 8 +++++--- 2 files changed, 16 insertions(+), 16 deletions(-)