Re: [Qemu-ppc] [Qemu-devel] [PATCH 02/14] target/ppc: remove getVSR()/putVSR() from mem_helper.c

[Mark Cave-Ayland]

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.