[Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Christophe Lyon]

Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

Also move the neon 64 bit saturating add/sub helpers to neon_helper.c
for consistency with the 32 bits versions.

There is probably still room for code commonalization though.

Peter, this patch is based upon your patch 6f83e7d and adds the 64 bits case.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helpers.h     |   12 ++++--
 target-arm/neon_helper.c |   89 ++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/op_helper.c   |   49 -------------------------
 target-arm/translate.c   |   18 ++++-----
 4 files changed, 105 insertions(+), 63 deletions(-)

diff --git a/target-arm/helpers.h b/target-arm/helpers.h
index b88ebae..8a2564e 100644
--- a/target-arm/helpers.h
+++ b/target-arm/helpers.h
@@ -137,10 +137,6 @@ DEF_HELPER_2(rsqrte_f32, f32, f32, env)
 DEF_HELPER_2(recpe_u32, i32, i32, env)
 DEF_HELPER_2(rsqrte_u32, i32, i32, env)
 DEF_HELPER_4(neon_tbl, i32, i32, i32, i32, i32)
-DEF_HELPER_2(neon_add_saturate_u64, i64, i64, i64)
-DEF_HELPER_2(neon_add_saturate_s64, i64, i64, i64)
-DEF_HELPER_2(neon_sub_saturate_u64, i64, i64, i64)
-DEF_HELPER_2(neon_sub_saturate_s64, i64, i64, i64)
 
 DEF_HELPER_2(add_cc, i32, i32, i32)
 DEF_HELPER_2(adc_cc, i32, i32, i32)
@@ -160,10 +156,18 @@ DEF_HELPER_3(neon_qadd_u8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qadd_s8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qadd_u16, i32, env, i32, i32)
 DEF_HELPER_3(neon_qadd_s16, i32, env, i32, i32)
+DEF_HELPER_3(neon_qadd_u32, i32, env, i32, i32)
+DEF_HELPER_3(neon_qadd_s32, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_u8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_s8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_u16, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_s16, i32, env, i32, i32)
+DEF_HELPER_3(neon_qsub_u32, i32, env, i32, i32)
+DEF_HELPER_3(neon_qsub_s32, i32, env, i32, i32)
+DEF_HELPER_3(neon_qadd_u64, i64, env, i64, i64)
+DEF_HELPER_3(neon_qadd_s64, i64, env, i64, i64)
+DEF_HELPER_3(neon_qsub_u64, i64, env, i64, i64)
+DEF_HELPER_3(neon_qsub_s64, i64, env, i64, i64)
 
 DEF_HELPER_2(neon_hadd_s8, i32, i32, i32)
 DEF_HELPER_2(neon_hadd_u8, i32, i32, i32)
diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 20f3c16..c1619c0 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -198,6 +198,28 @@ NEON_VOP_ENV(qadd_u16, neon_u16, 2)
 #undef NEON_FN
 #undef NEON_USAT
 
+uint32_t HELPER(neon_qadd_u32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a + b;
+    if (res < a) {
+        SET_QC();
+        res = ~0;
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qadd_u64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  res = src1 + src2;
+  if (res < src1) {
+    SET_QC();
+    res = ~(uint64_t)0;
+  }
+  return res;
+}
+
 #define NEON_SSAT(dest, src1, src2, type) do { \
     int32_t tmp = (uint32_t)src1 + (uint32_t)src2; \
     if (tmp != (type)tmp) { \
@@ -218,6 +240,28 @@ NEON_VOP_ENV(qadd_s16, neon_s16, 2)
 #undef NEON_FN
 #undef NEON_SSAT
 
+uint32_t HELPER(neon_qadd_s32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a + b;
+    if (((res ^ a) & SIGNBIT) && !((a ^ b) & SIGNBIT)) {
+        SET_QC();
+        res = ~(((int32_t)a >> 31) ^ SIGNBIT);
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qadd_s64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  res = src1 + src2;
+  if (((res ^ src1) & SIGNBIT64) && !((src1 ^ src2) & SIGNBIT64)) {
+    SET_QC();
+    res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
+  }
+  return res;
+}
+
 #define NEON_USAT(dest, src1, src2, type) do { \
     uint32_t tmp = (uint32_t)src1 - (uint32_t)src2; \
     if (tmp != (type)tmp) { \
@@ -234,6 +278,29 @@ NEON_VOP_ENV(qsub_u16, neon_u16, 2)
 #undef NEON_FN
 #undef NEON_USAT
 
+uint32_t HELPER(neon_qsub_u32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a - b;
+    if (res > a) {
+        SET_QC();
+        res = 0;
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qsub_u64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  if (src1 < src2) {
+    SET_QC();
+    res = 0;
+  } else {
+    res = src1 - src2;
+  }
+  return res;
+}
+
 #define NEON_SSAT(dest, src1, src2, type) do { \
     int32_t tmp = (uint32_t)src1 - (uint32_t)src2; \
     if (tmp != (type)tmp) { \
@@ -254,6 +321,28 @@ NEON_VOP_ENV(qsub_s16, neon_s16, 2)
 #undef NEON_FN
 #undef NEON_SSAT
 
+uint32_t HELPER(neon_qsub_s32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a - b;
+    if (((res ^ a) & SIGNBIT) && ((a ^ b) & SIGNBIT)) {
+        SET_QC();
+        res = ~(((int32_t)a >> 31) ^ SIGNBIT);
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qsub_s64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  res = src1 - src2;
+  if (((res ^ src1) & SIGNBIT64) && ((src1 ^ src2) & SIGNBIT64)) {
+    SET_QC();
+    res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
+  }
+  return res;
+}
+
 #define NEON_FN(dest, src1, src2) dest = (src1 + src2) >> 1
 NEON_VOP(hadd_s8, neon_s8, 4)
 NEON_VOP(hadd_u8, neon_u8, 4)
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 43baa63..3de2610 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -424,52 +424,3 @@ uint32_t HELPER(ror_cc)(uint32_t x, uint32_t i)
         return ((uint32_t)x >> shift) | (x << (32 - shift));
     }
 }
-
-uint64_t HELPER(neon_add_saturate_s64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    res = src1 + src2;
-    if (((res ^ src1) & SIGNBIT64) && !((src1 ^ src2) & SIGNBIT64)) {
-        env->QF = 1;
-        res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
-    }
-    return res;
-}
-
-uint64_t HELPER(neon_add_saturate_u64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    res = src1 + src2;
-    if (res < src1) {
-        env->QF = 1;
-        res = ~(uint64_t)0;
-    }
-    return res;
-}
-
-uint64_t HELPER(neon_sub_saturate_s64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    res = src1 - src2;
-    if (((res ^ src1) & SIGNBIT64) && ((src1 ^ src2) & SIGNBIT64)) {
-        env->QF = 1;
-        res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
-    }
-    return res;
-}
-
-uint64_t HELPER(neon_sub_saturate_u64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    if (src1 < src2) {
-        env->QF = 1;
-        res = 0;
-    } else {
-        res = src1 - src2;
-    }
-    return res;
-}
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 41cbb96..d4566f2 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -3539,12 +3539,6 @@ static inline void gen_neon_rsb(int size, TCGv t0, TCGv t1)
 #define gen_helper_neon_pmin_s32  gen_helper_neon_min_s32
 #define gen_helper_neon_pmin_u32  gen_helper_neon_min_u32
 
-/* FIXME: This is wrong.  They set the wrong overflow bit.  */
-#define gen_helper_neon_qadd_s32(a, e, b, c) gen_helper_add_saturate(a, b, c)
-#define gen_helper_neon_qadd_u32(a, e, b, c) gen_helper_add_usaturate(a, b, c)
-#define gen_helper_neon_qsub_s32(a, e, b, c) gen_helper_sub_saturate(a, b, c)
-#define gen_helper_neon_qsub_u32(a, e, b, c) gen_helper_sub_usaturate(a, b, c)
-
 #define GEN_NEON_INTEGER_OP_ENV(name) do { \
     switch ((size << 1) | u) { \
     case 0: \
@@ -4233,16 +4227,20 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 switch (op) {
                 case 1: /* VQADD */
                     if (u) {
-                        gen_helper_neon_add_saturate_u64(CPU_V001);
+                      gen_helper_neon_qadd_u64(cpu_V0, cpu_env,
+                                               cpu_V0, cpu_V1);
                     } else {
-                        gen_helper_neon_add_saturate_s64(CPU_V001);
+                      gen_helper_neon_qadd_s64(cpu_V0, cpu_env,
+                                               cpu_V0, cpu_V1);
                     }
                     break;
                 case 5: /* VQSUB */
                     if (u) {
-                        gen_helper_neon_sub_saturate_u64(CPU_V001);
+                      gen_helper_neon_qsub_u64(cpu_V0, cpu_env,
+                                               cpu_V0, cpu_V1);
                     } else {
-                        gen_helper_neon_sub_saturate_s64(CPU_V001);
+                      gen_helper_neon_qsub_s64(cpu_V0, cpu_env,
+                                               cpu_V0, cpu_V1);
                     }
                     break;
                 case 8: /* VSHL */
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Peter Maydell]

On 20 January 2011 17:16, Christophe Lyon <christophe.lyon@st.com> wrote:
> Set the right overflow bit for neon 32 and 64 bit saturating add/sub.
>
> Also move the neon 64 bit saturating add/sub helpers to neon_helper.c
> for consistency with the 32 bits versions.
>
> There is probably still room for code commonalization though.
>
> Peter, this patch is based upon your patch 6f83e7d and adds the 64 bits case.
>
> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

You shouldn't really leave my sign-off in there if you've changed the
patch. (Actually a lot of the changes in the tree I pointed you at have
my sign-off and really oughtn't -- all I was doing was splitting and
rearranging existing meego commits, so those are all unreviewed /
untested; so you should just remove it anyway.)

I'll review this tomorrow.

-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Peter Maydell]

On 20 January 2011 17:16, Christophe Lyon <christophe.lyon@st.com> wrote:
> Set the right overflow bit for neon 32 and 64 bit saturating add/sub.
>
> Also move the neon 64 bit saturating add/sub helpers to neon_helper.c
> for consistency with the 32 bits versions.
>
> There is probably still room for code commonalization though.
>
> Peter, this patch is based upon your patch 6f83e7d and adds the 64 bits case.

I've reviewed this patch and tested it in the usual way
and can confirm that it now sets the right saturation bit;
mostly it is OK. However...

> @@ -4233,16 +4227,20 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
>                 switch (op) {
>                 case 1: /* VQADD */
>                     if (u) {
> -                        gen_helper_neon_add_saturate_u64(CPU_V001);
> +                      gen_helper_neon_qadd_u64(cpu_V0, cpu_env,
> +                                               cpu_V0, cpu_V1);
>                     } else {
> -                        gen_helper_neon_add_saturate_s64(CPU_V001);
> +                      gen_helper_neon_qadd_s64(cpu_V0, cpu_env,
> +                                               cpu_V0, cpu_V1);
>                     }
>                     break;
>                 case 5: /* VQSUB */
>                     if (u) {
> -                        gen_helper_neon_sub_saturate_u64(CPU_V001);
> +                      gen_helper_neon_qsub_u64(cpu_V0, cpu_env,
> +                                               cpu_V0, cpu_V1);
>                     } else {
> -                        gen_helper_neon_sub_saturate_s64(CPU_V001);
> +                      gen_helper_neon_qsub_s64(cpu_V0, cpu_env,
> +                                               cpu_V0, cpu_V1);
>                     }
>                     break;
>                 case 8: /* VSHL */

the indentation in this hunk is wrong -- qemu standard is four-space.

You can check for this sort of thing by running scripts/checkpatch.pl,
which (as well as a pile of false positives because it's got confused
by the HELPER() macro) says:

WARNING: suspect code indent for conditional statements (20, 22)
#264: FILE: target-arm/translate.c:4229:
                     if (u) {
+                      gen_helper_neon_qadd_u64(cpu_V0, cpu_env,

WARNING: suspect code indent for conditional statements (20, 22)
#275: FILE: target-arm/translate.c:4238:
                     if (u) {
+                      gen_helper_neon_qsub_u64(cpu_V0, cpu_env,

-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Christophe Lyon]

On 21.01.2011 18:58, Peter Maydell wrote:
> I've reviewed this patch and tested it in the usual way
> and can confirm that it now sets the right saturation bit;
> mostly it is OK. However...
Thanks!

 
[...]
> the indentation in this hunk is wrong -- qemu standard is four-space.

Oops. What is the "official" emacs configuration to get the right qemu style?


> You can check for this sort of thing by running scripts/checkpatch.pl,
Thanks for the pointer.

Here is an updated patch with these minor fixes.

Christophe.


Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
---
 target-arm/helpers.h     |   12 ++++--
 target-arm/neon_helper.c |   89 ++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/op_helper.c   |   49 -------------------------
 target-arm/translate.c   |   18 ++++-----
 4 files changed, 105 insertions(+), 63 deletions(-)

diff --git a/target-arm/helpers.h b/target-arm/helpers.h
index b88ebae..8a2564e 100644
--- a/target-arm/helpers.h
+++ b/target-arm/helpers.h
@@ -137,10 +137,6 @@ DEF_HELPER_2(rsqrte_f32, f32, f32, env)
 DEF_HELPER_2(recpe_u32, i32, i32, env)
 DEF_HELPER_2(rsqrte_u32, i32, i32, env)
 DEF_HELPER_4(neon_tbl, i32, i32, i32, i32, i32)
-DEF_HELPER_2(neon_add_saturate_u64, i64, i64, i64)
-DEF_HELPER_2(neon_add_saturate_s64, i64, i64, i64)
-DEF_HELPER_2(neon_sub_saturate_u64, i64, i64, i64)
-DEF_HELPER_2(neon_sub_saturate_s64, i64, i64, i64)
 
 DEF_HELPER_2(add_cc, i32, i32, i32)
 DEF_HELPER_2(adc_cc, i32, i32, i32)
@@ -160,10 +156,18 @@ DEF_HELPER_3(neon_qadd_u8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qadd_s8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qadd_u16, i32, env, i32, i32)
 DEF_HELPER_3(neon_qadd_s16, i32, env, i32, i32)
+DEF_HELPER_3(neon_qadd_u32, i32, env, i32, i32)
+DEF_HELPER_3(neon_qadd_s32, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_u8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_s8, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_u16, i32, env, i32, i32)
 DEF_HELPER_3(neon_qsub_s16, i32, env, i32, i32)
+DEF_HELPER_3(neon_qsub_u32, i32, env, i32, i32)
+DEF_HELPER_3(neon_qsub_s32, i32, env, i32, i32)
+DEF_HELPER_3(neon_qadd_u64, i64, env, i64, i64)
+DEF_HELPER_3(neon_qadd_s64, i64, env, i64, i64)
+DEF_HELPER_3(neon_qsub_u64, i64, env, i64, i64)
+DEF_HELPER_3(neon_qsub_s64, i64, env, i64, i64)
 
 DEF_HELPER_2(neon_hadd_s8, i32, i32, i32)
 DEF_HELPER_2(neon_hadd_u8, i32, i32, i32)
diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 20f3c16..c1619c0 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -198,6 +198,28 @@ NEON_VOP_ENV(qadd_u16, neon_u16, 2)
 #undef NEON_FN
 #undef NEON_USAT
 
+uint32_t HELPER(neon_qadd_u32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a + b;
+    if (res < a) {
+        SET_QC();
+        res = ~0;
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qadd_u64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  res = src1 + src2;
+  if (res < src1) {
+    SET_QC();
+    res = ~(uint64_t)0;
+  }
+  return res;
+}
+
 #define NEON_SSAT(dest, src1, src2, type) do { \
     int32_t tmp = (uint32_t)src1 + (uint32_t)src2; \
     if (tmp != (type)tmp) { \
@@ -218,6 +240,28 @@ NEON_VOP_ENV(qadd_s16, neon_s16, 2)
 #undef NEON_FN
 #undef NEON_SSAT
 
+uint32_t HELPER(neon_qadd_s32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a + b;
+    if (((res ^ a) & SIGNBIT) && !((a ^ b) & SIGNBIT)) {
+        SET_QC();
+        res = ~(((int32_t)a >> 31) ^ SIGNBIT);
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qadd_s64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  res = src1 + src2;
+  if (((res ^ src1) & SIGNBIT64) && !((src1 ^ src2) & SIGNBIT64)) {
+    SET_QC();
+    res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
+  }
+  return res;
+}
+
 #define NEON_USAT(dest, src1, src2, type) do { \
     uint32_t tmp = (uint32_t)src1 - (uint32_t)src2; \
     if (tmp != (type)tmp) { \
@@ -234,6 +278,29 @@ NEON_VOP_ENV(qsub_u16, neon_u16, 2)
 #undef NEON_FN
 #undef NEON_USAT
 
+uint32_t HELPER(neon_qsub_u32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a - b;
+    if (res > a) {
+        SET_QC();
+        res = 0;
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qsub_u64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  if (src1 < src2) {
+    SET_QC();
+    res = 0;
+  } else {
+    res = src1 - src2;
+  }
+  return res;
+}
+
 #define NEON_SSAT(dest, src1, src2, type) do { \
     int32_t tmp = (uint32_t)src1 - (uint32_t)src2; \
     if (tmp != (type)tmp) { \
@@ -254,6 +321,28 @@ NEON_VOP_ENV(qsub_s16, neon_s16, 2)
 #undef NEON_FN
 #undef NEON_SSAT
 
+uint32_t HELPER(neon_qsub_s32)(CPUState *env, uint32_t a, uint32_t b)
+{
+    uint32_t res = a - b;
+    if (((res ^ a) & SIGNBIT) && ((a ^ b) & SIGNBIT)) {
+        SET_QC();
+        res = ~(((int32_t)a >> 31) ^ SIGNBIT);
+    }
+    return res;
+}
+
+uint64_t HELPER(neon_qsub_s64)(CPUState *env, uint64_t src1, uint64_t src2)
+{
+  uint64_t res;
+
+  res = src1 - src2;
+  if (((res ^ src1) & SIGNBIT64) && ((src1 ^ src2) & SIGNBIT64)) {
+    SET_QC();
+    res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
+  }
+  return res;
+}
+
 #define NEON_FN(dest, src1, src2) dest = (src1 + src2) >> 1
 NEON_VOP(hadd_s8, neon_s8, 4)
 NEON_VOP(hadd_u8, neon_u8, 4)
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 43baa63..3de2610 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -424,52 +424,3 @@ uint32_t HELPER(ror_cc)(uint32_t x, uint32_t i)
         return ((uint32_t)x >> shift) | (x << (32 - shift));
     }
 }
-
-uint64_t HELPER(neon_add_saturate_s64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    res = src1 + src2;
-    if (((res ^ src1) & SIGNBIT64) && !((src1 ^ src2) & SIGNBIT64)) {
-        env->QF = 1;
-        res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
-    }
-    return res;
-}
-
-uint64_t HELPER(neon_add_saturate_u64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    res = src1 + src2;
-    if (res < src1) {
-        env->QF = 1;
-        res = ~(uint64_t)0;
-    }
-    return res;
-}
-
-uint64_t HELPER(neon_sub_saturate_s64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    res = src1 - src2;
-    if (((res ^ src1) & SIGNBIT64) && ((src1 ^ src2) & SIGNBIT64)) {
-        env->QF = 1;
-        res = ((int64_t)src1 >> 63) ^ ~SIGNBIT64;
-    }
-    return res;
-}
-
-uint64_t HELPER(neon_sub_saturate_u64)(uint64_t src1, uint64_t src2)
-{
-    uint64_t res;
-
-    if (src1 < src2) {
-        env->QF = 1;
-        res = 0;
-    } else {
-        res = src1 - src2;
-    }
-    return res;
-}
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 41cbb96..ab4aa29 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -3539,12 +3539,6 @@ static inline void gen_neon_rsb(int size, TCGv t0, TCGv t1)
 #define gen_helper_neon_pmin_s32  gen_helper_neon_min_s32
 #define gen_helper_neon_pmin_u32  gen_helper_neon_min_u32
 
-/* FIXME: This is wrong.  They set the wrong overflow bit.  */
-#define gen_helper_neon_qadd_s32(a, e, b, c) gen_helper_add_saturate(a, b, c)
-#define gen_helper_neon_qadd_u32(a, e, b, c) gen_helper_add_usaturate(a, b, c)
-#define gen_helper_neon_qsub_s32(a, e, b, c) gen_helper_sub_saturate(a, b, c)
-#define gen_helper_neon_qsub_u32(a, e, b, c) gen_helper_sub_usaturate(a, b, c)
-
 #define GEN_NEON_INTEGER_OP_ENV(name) do { \
     switch ((size << 1) | u) { \
     case 0: \
@@ -4233,16 +4227,20 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                 switch (op) {
                 case 1: /* VQADD */
                     if (u) {
-                        gen_helper_neon_add_saturate_u64(CPU_V001);
+                        gen_helper_neon_qadd_u64(cpu_V0, cpu_env,
+                                                 cpu_V0, cpu_V1);
                     } else {
-                        gen_helper_neon_add_saturate_s64(CPU_V001);
+                        gen_helper_neon_qadd_s64(cpu_V0, cpu_env,
+                                                 cpu_V0, cpu_V1);
                     }
                     break;
                 case 5: /* VQSUB */
                     if (u) {
-                        gen_helper_neon_sub_saturate_u64(CPU_V001);
+                        gen_helper_neon_qsub_u64(cpu_V0, cpu_env,
+                                                 cpu_V0, cpu_V1);
                     } else {
-                        gen_helper_neon_sub_saturate_s64(CPU_V001);
+                        gen_helper_neon_qsub_s64(cpu_V0, cpu_env,
+                                                 cpu_V0, cpu_V1);
                     }
                     break;
                 case 8: /* VSHL */
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Peter Maydell]

On 24 January 2011 12:41, Christophe Lyon <christophe.lyon@st.com> wrote:
> On 21.01.2011 18:58, Peter Maydell wrote:
>> the indentation in this hunk is wrong -- qemu standard is four-space.
>
> Oops. What is the "official" emacs configuration to get the right qemu style?

I don't know about official, but what I use is this:
https://wiki.linaro.org/PeterMaydell/QemuEmacsStyle

-- PMM

Re: [Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Christophe Lyon]

On 24.01.2011 13:41, Christophe Lyon wrote:
> Here is an updated patch with these minor fixes.
> 

ping?

Re: [Qemu-devel] [PATCH] target-arm: Set the right overflow bit for neon 32 and 64 bit saturating add/sub.

[Peter Maydell]

On 24 January 2011 12:41, Christophe Lyon <christophe.lyon@st.com> wrote:
> Here is an updated patch with these minor fixes.

> +uint64_t HELPER(neon_qadd_u64)(CPUState *env, uint64_t src1, uint64_t src2)
> +{
> +  uint64_t res;
> +
> +  res = src1 + src2;
> +  if (res < src1) {
> +    SET_QC();
> +    res = ~(uint64_t)0;
> +  }
> +  return res;
> +}

Indentation is still wrong here (not sure why checkpatch didn't
spot it) and in the other s64 and u64 helpers. Sorry I didn't
notice that last time round.

When you resubmit v3 can you make sure you use the right
email format to give a sensible git commit message, please?
(ie random commentary below the '---' so it doesn't go in
the git changelog.)

Thanks
-- PMM