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

[Mark Cave-Ayland]

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.