[1/7] target/ppc: introduce get_dfp{64,128}() helper functions

The existing functions (now incorrectly) assume that the MSB and LSB of DFP
numbers are stored as consecutive 64-bit words in memory. Instead of accessing
the DFP numbers directly, introduce get_dfp{64,128}() helper functions to ease
the switch to the correct representation.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 target/ppc/dfp_helper.c | 52 ++++++++++++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 13 deletions(-)

On 9/24/19 8:35 AM, Mark Cave-Ayland wrote:
> +static void get_dfp64(uint64_t *dst, uint64_t *dfp)
> +{
> +    dst[0] = dfp[0];
> +}
> +
> +static void get_dfp128(uint64_t *dst, uint64_t *dfp)
> +{
> +    dst[0] = dfp[HI_IDX];
> +    dst[1] = dfp[LO_IDX];
> +}

I'm not keen on this.  I would prefer some type difference that prevents you
from getting the arguments the wrong way around.

I'm thinking that a combination helper like

static void get_dfp64(decNumber *out, uint64_t *in)
{
    union {
       uint64_t i;
       decimal64 d;
    } u;

    u.i = *in;
    decimal64ToNumber(&u.d, out);
}

> @@ -129,7 +140,7 @@ static void dfp_prepare_decimal64(struct PPC_DFP *dfp, uint64_t *a,
>      dfp->env = env;
>  
>      if (a) {
> -        dfp->a64[0] = *a;
> +        get_dfp64(dfp->a64, a);
>          decimal64ToNumber((decimal64 *)dfp->a64, &dfp->a);
>      } else {
>          dfp->a64[0] = 0;

becomes

    get_dfp64(&dfp->a, a);

and that might clean up a lot of the code.

> @@ -617,10 +626,12 @@ uint32_t helper_##op(CPUPPCState *env, uint64_t *a, uint64_t *b)         \
>  {                                                                        \
>      struct PPC_DFP dfp;                                                  \
>      unsigned k;                                                          \
> +    uint64_t a64;                                                        \
>                                                                           \
>      dfp_prepare_decimal##size(&dfp, 0, b, env);                          \
>                                                                           \
> -    k = *a & 0x3F;                                                       \
> +    get_dfp64(&a64, a);                                                  \
> +    k = a64 & 0x3F;                                                      \
>                                                                           \
>      if (unlikely(decNumberIsSpecial(&dfp.b))) {                          \
>          dfp.crbf = 1;                                                    \

Whereas cases like this, where a scalar is being passed in, I don't think that
re-using the same helper is appropriate.  Ideally, they would be passed in by
value, and not by reference at all.  That, of course, requires changes to the
translator beyond the scope of this patch.

>  void helper_dctqpq(CPUPPCState *env, uint64_t *t, uint64_t *b)
>  {
>      struct PPC_DFP dfp;
> +    uint64_t b64;
>      dfp_prepare_decimal128(&dfp, 0, 0, env);
> -    decimal64ToNumber((decimal64 *)b, &dfp.t);
> +    get_dfp64(&b64, b);
> +    decimal64ToNumber((decimal64 *)&b64, &dfp.t);

This would become

    get_dfp64(&dfp.t, b);

with no intermediate b64 temp.


r~

On 24/09/2019 20:21, Richard Henderson wrote:

> On 9/24/19 8:35 AM, Mark Cave-Ayland wrote:
>> +static void get_dfp64(uint64_t *dst, uint64_t *dfp)
>> +{
>> +    dst[0] = dfp[0];
>> +}
>> +
>> +static void get_dfp128(uint64_t *dst, uint64_t *dfp)
>> +{
>> +    dst[0] = dfp[HI_IDX];
>> +    dst[1] = dfp[LO_IDX];
>> +}
> 
> I'm not keen on this.  I would prefer some type difference that prevents you
> from getting the arguments the wrong way around.
> 
> I'm thinking that a combination helper like
> 
> static void get_dfp64(decNumber *out, uint64_t *in)
> {
>     union {
>        uint64_t i;
>        decimal64 d;
>     } u;
> 
>     u.i = *in;
>     decimal64ToNumber(&u.d, out);
> }
> 
>> @@ -129,7 +140,7 @@ static void dfp_prepare_decimal64(struct PPC_DFP *dfp, uint64_t *a,
>>      dfp->env = env;
>>  
>>      if (a) {
>> -        dfp->a64[0] = *a;
>> +        get_dfp64(dfp->a64, a);
>>          decimal64ToNumber((decimal64 *)dfp->a64, &dfp->a);
>>      } else {
>>          dfp->a64[0] = 0;
> 
> becomes
> 
>     get_dfp64(&dfp->a, a);
> 
> and that might clean up a lot of the code.

Right, and in fact I had a similar thought myself regarding type safety since one of
the issues with working with the existing code in dfp_helper.c is that everything
uses a uint64_t * which makes it really difficult to figure out why things are
crashing if you make a typo :/

Note that this patch simply introduces the helpers without making changes to the
existing logic which is why both arguments are still uint64_t *. The real work is
done in patch 3 where ppc_fptr_t type is introduced, and also see the switch from
uint64_t * to ppc_vsr_t in patch 5.

With the full patchset applied you'll see that get_dfp64() and friends are in exactly
the same form you show above, and if I swap the arguments then the compiler does
actually complain, although somewhat cryptically.

>> @@ -617,10 +626,12 @@ uint32_t helper_##op(CPUPPCState *env, uint64_t *a, uint64_t *b)         \
>>  {                                                                        \
>>      struct PPC_DFP dfp;                                                  \
>>      unsigned k;                                                          \
>> +    uint64_t a64;                                                        \
>>                                                                           \
>>      dfp_prepare_decimal##size(&dfp, 0, b, env);                          \
>>                                                                           \
>> -    k = *a & 0x3F;                                                       \
>> +    get_dfp64(&a64, a);                                                  \
>> +    k = a64 & 0x3F;                                                      \
>>                                                                           \
>>      if (unlikely(decNumberIsSpecial(&dfp.b))) {                          \
>>          dfp.crbf = 1;                                                    \
> 
> Whereas cases like this, where a scalar is being passed in, I don't think that
> re-using the same helper is appropriate.  Ideally, they would be passed in by
> value, and not by reference at all.  That, of course, requires changes to the
> translator beyond the scope of this patch.

Agreed. I was really keen to avoid any translator changes if possible since the PPC
translator code tends to be quite tricky which is why I went with the above approach.

>>  void helper_dctqpq(CPUPPCState *env, uint64_t *t, uint64_t *b)
>>  {
>>      struct PPC_DFP dfp;
>> +    uint64_t b64;
>>      dfp_prepare_decimal128(&dfp, 0, 0, env);
>> -    decimal64ToNumber((decimal64 *)b, &dfp.t);
>> +    get_dfp64(&b64, b);
>> +    decimal64ToNumber((decimal64 *)&b64, &dfp.t);
> 
> This would become
> 
>     get_dfp64(&dfp.t, b);
> 
> with no intermediate b64 temp.

Funnily enough that did cross my mind, but I wasn't 100% sure that this wasn't doing
something extra within libdecnumber. If you think it makes sense then I can easily
add it into a v2.


ATB,

Mark.

On 9/24/19 2:05 PM, Mark Cave-Ayland wrote:
> With the full patchset applied you'll see that get_dfp64() and friends are
> in exactly the same form you show above, and if I swap the arguments then
> the compiler does actually complain, although somewhat cryptically.
Oh, good.  I'll finish reading the whole set before making too many more
comments ahead of your actual steps.


r~