qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 6/6] target-arm: Implement ARMv8 SIMD VMAXNM


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v7 6/6] target-arm: Implement ARMv8 SIMD VMAXNM and VMINNM instructions.
Date: Mon, 2 Dec 2013 22:13:03 +0000

On 2 December 2013 20:12, Will Newton <address@hidden> wrote:
> This adds support for the ARMv8 Advanced SIMD VMAXNM and VMINNM
> instructions.
>
> Signed-off-by: Will Newton <address@hidden>
> ---
>  target-arm/helper.h      |  3 +++
>  target-arm/neon_helper.c | 16 ++++++++++++++++
>  target-arm/translate.c   | 31 ++++++++++++++++++++++---------
>  3 files changed, 41 insertions(+), 9 deletions(-)
>
> Changes in v7:
>  - Use new softfloat routines for minnum/maxnum
>  - Rename NEON_3R_VRECPS_VRSQRTS to NEON_3R_FLOAT_MISC
>  - Fix brace style
>
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index d459a39..3ecbbd2 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -355,6 +355,9 @@ DEF_HELPER_3(neon_cgt_f32, i32, i32, i32, ptr)
>  DEF_HELPER_3(neon_acge_f32, i32, i32, i32, ptr)
>  DEF_HELPER_3(neon_acgt_f32, i32, i32, i32, ptr)
>
> +DEF_HELPER_3(neon_maxnm_f32, i32, i32, i32, ptr)
> +DEF_HELPER_3(neon_minnm_f32, i32, i32, i32, ptr)
> +
>  /* iwmmxt_helper.c */
>  DEF_HELPER_2(iwmmxt_maddsq, i64, i64, i64)
>  DEF_HELPER_2(iwmmxt_madduq, i64, i64, i64)
> diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
> index b028cc2..06e6894 100644
> --- a/target-arm/neon_helper.c
> +++ b/target-arm/neon_helper.c
> @@ -2008,3 +2008,19 @@ void HELPER(neon_zip16)(CPUARMState *env, uint32_t rd, 
> uint32_t rm)
>      env->vfp.regs[rm] = make_float64(m0);
>      env->vfp.regs[rd] = make_float64(d0);
>  }
> +
> +uint32_t HELPER(neon_maxnm_f32)(uint32_t a, uint32_t b, void *fpstp)
> +{
> +    float_status *fpst = fpstp;
> +    float32 af = make_float32(a);
> +    float32 bf = make_float32(b);
> +    return float32_val(float32_maxnum(af, bf, fpst));
> +}
> +
> +uint32_t HELPER(neon_minnm_f32)(uint32_t a, uint32_t b, void *fpstp)
> +{
> +    float_status *fpst = fpstp;
> +    float32 af = make_float32(a);
> +    float32 bf = make_float32(b);
> +    return float32_val(float32_minnum(af, bf, fpst));
> +}

These are still unnecessary duplicates of the helpers you've put in in
patch 5 -- just use those from the Neon instructions as well as the
VFP ones. The float32_val and make_float32 boxing/unboxing
macros are only so we get better typechecking from the C compiler,
so it is exactly equivalent to define the helper as taking and returning
float32, or to have it take and return uint32_t and then do the box/unbox
in the function. Just having it take/return float32 is shorter and easier
to read.

> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 9a8069e..7d9213e 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4553,7 +4553,7 @@ static void gen_neon_narrow_op(int op, int u, int size,
>  #define NEON_3R_FLOAT_CMP 28 /* float VCEQ, VCGE, VCGT */
>  #define NEON_3R_FLOAT_ACMP 29 /* float VACGE, VACGT, VACLE, VACLT */
>  #define NEON_3R_FLOAT_MINMAX 30 /* float VMIN, VMAX */
> -#define NEON_3R_VRECPS_VRSQRTS 31 /* float VRECPS, VRSQRTS */
> +#define NEON_3R_FLOAT_MISC 31 /* float VRECPS, VRSQRTS, VMAXNM/MINNM */
>
>  static const uint8_t neon_3r_sizes[] = {
>      [NEON_3R_VHADD] = 0x7,
> @@ -4586,7 +4586,7 @@ static const uint8_t neon_3r_sizes[] = {
>      [NEON_3R_FLOAT_CMP] = 0x5, /* size bit 1 encodes op */
>      [NEON_3R_FLOAT_ACMP] = 0x5, /* size bit 1 encodes op */
>      [NEON_3R_FLOAT_MINMAX] = 0x5, /* size bit 1 encodes op */
> -    [NEON_3R_VRECPS_VRSQRTS] = 0x5, /* size bit 1 encodes op */
> +    [NEON_3R_FLOAT_MISC] = 0x5, /* size bit 1 encodes op */
>  };
>
>  /* Symbolic constants for op fields for Neon 2-register miscellaneous.
> @@ -4847,8 +4847,9 @@ static int disas_neon_data_insn(CPUARMState * env, 
> DisasContext *s, uint32_t ins
>                  return 1;
>              }
>              break;
> -        case NEON_3R_VRECPS_VRSQRTS:
> -            if (u) {
> +        case NEON_3R_FLOAT_MISC:
> +            /* VMAXNM/VMINNM in ARMv8 */
> +            if (u && (!arm_feature(env, ARM_FEATURE_V8) || (size & 1))) {

I don't think we need to check size bit 0 here, do we?
The check against the entry in neon_3r_sizes[] above should
only pass if size is 0 or 2.

>                  return 1;
>              }
>              break;
> @@ -5137,11 +5138,23 @@ static int disas_neon_data_insn(CPUARMState * env, 
> DisasContext *s, uint32_t ins
>              tcg_temp_free_ptr(fpstatus);
>              break;
>          }
> -        case NEON_3R_VRECPS_VRSQRTS:
> -            if (size == 0)
> -                gen_helper_recps_f32(tmp, tmp, tmp2, cpu_env);
> -            else
> -                gen_helper_rsqrts_f32(tmp, tmp, tmp2, cpu_env);
> +        case NEON_3R_FLOAT_MISC:
> +            if (u) {
> +                /* VMAXNM/VMINNM */
> +                TCGv_ptr fpstatus = get_fpstatus_ptr(1);
> +                if (size == 0) {
> +                    gen_helper_neon_maxnm_f32(tmp, tmp, tmp2, fpstatus);
> +                } else {
> +                    gen_helper_neon_minnm_f32(tmp, tmp, tmp2, fpstatus);
> +                }
> +                tcg_temp_free_ptr(fpstatus);
> +            } else {
> +                if (size == 0) {
> +                    gen_helper_recps_f32(tmp, tmp, tmp2, cpu_env);
> +                } else {
> +                    gen_helper_rsqrts_f32(tmp, tmp, tmp2, cpu_env);
> +              }
> +            }
>              break;
>          case NEON_3R_VFM:
>          {

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]