Re: [Qemu-devel] [PATCH 4/8] target/ppc: introduce avrh_offset() and avrl_offset() functions

[Richard Henderson]

On 3/5/19 9:38 AM, Mark Cave-Ayland wrote:
> It's really that this is a stepping stone to patch 7 where you end up with 
> this:
> 
> static inline int vsrh_offset(int i)
> {
>     return offsetof(CPUPPCState, vsr[i].VsrD(0));
> }
> 
> static inline int vsrl_offset(int i)
> {
>     return offsetof(CPUPPCState, vsr[i].VsrD(1));
> }
> 
> ...
> 
> static inline int avrh_offset(int i)
> {
>     return vsrh_offset(i + 32);
> }
> 
> static inline int avrl_offset(int i)
> {
>     return vsrl_offset(i + 32);
> }

Yes, but the only users look like...

>     tcg_gen_ld_i64(dst, cpu_env, high ? avrh_offset(regno) : 
> avrl_offset(regno));

... this.  And to me that suggests that one helper instead of two would be
better, so that you can write

  tcg_gen_ld_i64(dst, cpu_env, avr64_offset(regno, high));

and propagate the "high" into the VsrD argument.

> which looks a bit easier on the eye? I'm less keen on pushing the "high" bool 
> all the
> way down into offset functions as I find the separate vsrh/vsrl functions 
> much easier
> to read in the helpers than the get_avr64() version.

Ah.  Hmm.

Will we not in the end require a function A64's

  vec_reg_offset(regno, element, size)

I know DavidH did when writing the s390x vector support last week.  Perhaps
that's more palatable than avr64_offset that only supports 64-bit elements?


r~