Re: [Qemu-devel] [PATCH v2 06/11] target-arm: Implement fcsel with movcond

[Peter Maydell]

On 2 September 2015 at 18:57, Richard Henderson <address@hidden> wrote:
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  target-arm/translate-a64.c | 48 
> ++++++++++++++++++++--------------------------
>  1 file changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 48ecf23..a6e5ccd 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -4168,20 +4168,6 @@ static void disas_fp_ccomp(DisasContext *s, uint32_t 
> insn)
>      }
>  }
>
> -/* copy src FP register to dst FP register; type specifies single or double 
> */
> -static void gen_mov_fp2fp(DisasContext *s, int type, int dst, int src)
> -{
> -    if (type) {
> -        TCGv_i64 v = read_fp_dreg(s, src);
> -        write_fp_dreg(s, dst, v);
> -        tcg_temp_free_i64(v);
> -    } else {
> -        TCGv_i32 v = read_fp_sreg(s, src);
> -        write_fp_sreg(s, dst, v);
> -        tcg_temp_free_i32(v);
> -    }
> -}
> -
>  /* C3.6.24 Floating point conditional select
>   *   31  30  29 28       24 23  22  21 20  16 15  12 11 10 9    5 4    0
>   * +---+---+---+-----------+------+---+------+------+-----+------+------+
> @@ -4191,7 +4177,8 @@ static void gen_mov_fp2fp(DisasContext *s, int type, 
> int dst, int src)
>  static void disas_fp_csel(DisasContext *s, uint32_t insn)
>  {
>      unsigned int mos, type, rm, cond, rn, rd;
> -    TCGLabel *label_continue = NULL;
> +    TCGv_i64 t_true, t_false, t_zero;
> +    DisasCompare64 c;
>
>      mos = extract32(insn, 29, 3);
>      type = extract32(insn, 22, 2); /* 0 = single, 1 = double */
> @@ -4209,21 +4196,28 @@ static void disas_fp_csel(DisasContext *s, uint32_t 
> insn)
>          return;
>      }
>
> -    if (cond < 0x0e) { /* not always */
> -        TCGLabel *label_match = gen_new_label();
> -        label_continue = gen_new_label();
> -        arm_gen_test_cc(cond, label_match);
> -        /* nomatch: */
> -        gen_mov_fp2fp(s, type, rd, rm);
> -        tcg_gen_br(label_continue);
> -        gen_set_label(label_match);
> +    if (type) {
> +        t_true = read_fp_dreg(s, rn);
> +        t_false = read_fp_dreg(s, rm);
> +    } else {
> +        /* Zero-extend sreg inputs to 64-bits now.  */
> +        t_true = tcg_temp_new_i64();
> +        t_false = tcg_temp_new_i64();
> +        tcg_gen_ld32u_i64(t_true, cpu_env, fp_reg_offset(s, rn, MO_32));
> +        tcg_gen_ld32u_i64(t_false, cpu_env, fp_reg_offset(s, rm, MO_32));

You could write these as
    read_vec_element(s, t_true, rn, 0, MO_32);
    read_vec_element(s, t_false, rm, 0, MO_32);

(ie "read the 0th element of size MO_32 from this vector register").
I'm on the fence about whether that's actually any clearer, though.
I suppose it does let you do

   read_vec_element(s, t_tre, rn, 0, type ? MO_64 : MO_32);
&c
and avoid the if (type)...

So you could change it, or leave it as-is, whichever you prefer.

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM