These will become more useful later, but initially use this as an aid to
simplify the offset calculation by replacing the HOST_TARGET_BIGENDIAN
sections with the VsrD macro.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/ppc/cpu.h       | 10 ++++++++++
 target/ppc/translate.c | 24 ++++++++++--------------
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index d0580c6b6d..326593e0e7 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2603,6 +2603,16 @@ static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
     return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
 }
 
+static inline int avrh_offset(int i)
+{
+    return offsetof(CPUPPCState, vsr[32 + i].VsrD(0));
+}
+
+static inline int avrl_offset(int i)
+{
+    return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
+}
+
 static inline ppc_avr_t *cpu_avr_ptr(CPUPPCState *env, int i)
 {
     return &env->vsr[32 + i];
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 3b1992faf1..f646f359e7 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -6687,24 +6687,20 @@ static inline void set_fpr(int regno, TCGv_i64 src)
 
 static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 0 : 1)]));
-#else
-    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 1 : 0)]));
-#endif
+    if (high) {
+        tcg_gen_ld_i64(dst, cpu_env, avrh_offset(regno));
+    } else {
+        tcg_gen_ld_i64(dst, cpu_env, avrl_offset(regno));
+    }
 }
 
 static inline void set_avr64(int regno, TCGv_i64 src, bool high)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 0 : 1)]));
-#else
-    tcg_gen_st_i64(src, cpu_env, offsetof(CPUPPCState,
-                                          vsr[32 + regno].u64[(high ? 1 : 0)]));
-#endif
+    if (high) {
+        tcg_gen_st_i64(src, cpu_env, avrh_offset(regno));
+    } else {
+        tcg_gen_st_i64(src, cpu_env, avrl_offset(regno));
+    }
 }
 
 #include "translate/fp-impl.inc.c"
-- 
2.11.0

On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
> These will become more useful later, but initially use this as an aid to
> simplify the offset calculation by replacing the HOST_TARGET_BIGENDIAN
> sections with the VsrD macro.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  target/ppc/cpu.h       | 10 ++++++++++
>  target/ppc/translate.c | 24 ++++++++++--------------
>  2 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d0580c6b6d..326593e0e7 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2603,6 +2603,16 @@ static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
>      return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
>  }
>  
> +static inline int avrh_offset(int i)
> +{
> +    return offsetof(CPUPPCState, vsr[32 + i].VsrD(0));
> +}
> +
> +static inline int avrl_offset(int i)
> +{
> +    return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
> +}

I really don't see the point of these...

>  static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
>  {
> -#ifdef HOST_WORDS_BIGENDIAN
> -    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
> -                                          vsr[32 + regno].u64[(high ? 0 : 1)]));
> -#else
> -    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
> -                                          vsr[32 + regno].u64[(high ? 1 : 0)]));
> -#endif
> +    if (high) {
> +        tcg_gen_ld_i64(dst, cpu_env, avrh_offset(regno));
> +    } else {
> +        tcg_gen_ld_i64(dst, cpu_env, avrl_offset(regno));
> +    }

When you could just as well write this as

  tcg_gen_ld_i64(dst, cpu_env,
    offsetof(CPUPPCState, vsr[32+regno].VsrD(high)));


r~

On 03/03/2019 23:31, Richard Henderson wrote:

> On 3/3/19 9:23 AM, Mark Cave-Ayland wrote:
>> These will become more useful later, but initially use this as an aid to
>> simplify the offset calculation by replacing the HOST_TARGET_BIGENDIAN
>> sections with the VsrD macro.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  target/ppc/cpu.h       | 10 ++++++++++
>>  target/ppc/translate.c | 24 ++++++++++--------------
>>  2 files changed, 20 insertions(+), 14 deletions(-)
>>
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index d0580c6b6d..326593e0e7 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -2603,6 +2603,16 @@ static inline uint64_t *cpu_vsrl_ptr(CPUPPCState *env, int i)
>>      return (uint64_t *)((uintptr_t)env + vsrl_offset(i));
>>  }
>>  
>> +static inline int avrh_offset(int i)
>> +{
>> +    return offsetof(CPUPPCState, vsr[32 + i].VsrD(0));
>> +}
>> +
>> +static inline int avrl_offset(int i)
>> +{
>> +    return offsetof(CPUPPCState, vsr[32 + i].VsrD(1));
>> +}
> 
> I really don't see the point of these...
> 
>>  static inline void get_avr64(TCGv_i64 dst, int regno, bool high)
>>  {
>> -#ifdef HOST_WORDS_BIGENDIAN
>> -    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
>> -                                          vsr[32 + regno].u64[(high ? 0 : 1)]));
>> -#else
>> -    tcg_gen_ld_i64(dst, cpu_env, offsetof(CPUPPCState,
>> -                                          vsr[32 + regno].u64[(high ? 1 : 0)]));
>> -#endif
>> +    if (high) {
>> +        tcg_gen_ld_i64(dst, cpu_env, avrh_offset(regno));
>> +    } else {
>> +        tcg_gen_ld_i64(dst, cpu_env, avrl_offset(regno));
>> +    }
> 
> When you could just as well write this as
> 
>   tcg_gen_ld_i64(dst, cpu_env,
>     offsetof(CPUPPCState, vsr[32+regno].VsrD(high)));

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);
}

i.e. the logic that describes the AVR registers as being the last set of 32 VSX
registers is captured more succinctly in the avr[l,h] wrapper functions. How about
rewriting the above like this:

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

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.


ATB,

Mark.

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~