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

[Richard Henderson <richard.henderson@linaro.org>]

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~