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.
