Re: [Qemu-devel] [RFC PATCH 1/6] target/ppc: introduce get_fpr() and set_fpr() helpers for FP register access

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

On 12/7/18 2:56 AM, Mark Cave-Ayland wrote:
> -    gen_helper_f##op(cpu_fpr[rD(ctx->opcode)], cpu_env,                       \
> -                     cpu_fpr[rA(ctx->opcode)],                                \
> -                     cpu_fpr[rC(ctx->opcode)], cpu_fpr[rB(ctx->opcode)]);     \
> +    get_fpr(t0, rA(ctx->opcode));                                             \
> +    get_fpr(t1, rC(ctx->opcode));                                             \
> +    get_fpr(t2, rB(ctx->opcode));                                             \
> +    gen_helper_f##op(t3, cpu_env, t0, t1, t2);                                \
> +    set_fpr(rD(ctx->opcode), t3);                                             \
>      if (isfloat) {                                                            \
> -        gen_helper_frsp(cpu_fpr[rD(ctx->opcode)], cpu_env,                    \
> -                        cpu_fpr[rD(ctx->opcode)]);                            \
> +        get_fpr(t0, rD(ctx->opcode));                                         \
> +        gen_helper_frsp(t3, cpu_env, t0);                                     \
> +        set_fpr(rD(ctx->opcode), t3);                                         \
>      }                                                                         \

This is an accurate conversion, but the writeback to the rD register need not
happen until after helper_frsp.  Just move it below the isfloat block.

I do see that helper_frsp can raise an exception for invalid_op for SNaN.  If
that code were actually reachable, this would have been an existing bug, in
that we should not have written back to rD after the first operation.  However,
any SNaN will already have been eliminated by the first operation (via
squashing to QNaN or by exiting via exception).

Similarly in GEN_FLOAT_AB.

> +    get_fpr(t0, rB(ctx->opcode));
> +    gen_helper_frsqrte(t1, cpu_env, t0);
> +    set_fpr(rD(ctx->opcode), t1);
> +    gen_helper_frsp(t1, cpu_env, t1);
> +    gen_compute_fprf_float64(t1);

gen_frsqrtes has the set_fpr in the wrong place.  Likewise gen_fsqrts.


r~