[Qemu-devel] [PATCH 00/10] Fix Neon shift instructions

[Qemu-devel] [PATCH 00/10] Fix Neon shift instructions

[Peter Maydell]

This patch series fixes bugs in the Neon shift instructions
VRSHR, VRSRA, VQRSHRN, VQRSHRUN, VRSHRN, VQSHRN, VSHRN, VQSHRUN,
VRSHL, and VQRSHL. It is based on the v3 patchset Christophe
sent recently, with some fixes for minor nits in those patches,
plus some patches from me which fix problems with shifts by
large shift counts and an issue with overlapping source and
destination registers.

With this patchset qemu passes random instruction sequence
testing for all these instruction patterns.

Christophe Lyon (5):
  target-arm: Fix rounding constant addition for Neon shifts
  target-arm: Fix unsigned VRSHL.s8 and .s16 right shifts by type width
  target-arm: fix unsigned 64 bit right shifts.
  target-arm: fix Neon VQSHRN and VSHRN.
  target-arm: fix decoding of Neon 64 bit shifts.

Peter Maydell (5):
  target-arm: Fix signed VRSHL by large shift counts
  target-arm: Fix saturated values for Neon right shifts
  target-arm: Fix signed VQRSHL by large shift counts
  target-arm: Fix unsigned VQRSHL by large shift counts
  target-arm: Fix shift by immediate and narrow where src,dest overlap

 target-arm/neon_helper.c |  232 +++++++++++++++++++++++++++++++++++++++-------
 target-arm/translate.c   |   71 ++++++++++----
 2 files changed, 250 insertions(+), 53 deletions(-)

[Qemu-devel] [PATCH 01/10] target-arm: Fix rounding constant addition for Neon shifts

[Peter Maydell]

From: Christophe Lyon <christophe.lyon@st.com>

Handle cases where adding the rounding constant could overflow in Neon
shift instructions: VRSHR, VRSRA, VQRSHRN, VQRSHRUN, VRSHRN.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
[peter.maydell@linaro.org: fix handling of large shifts in rshl_s32,
calculate signed saturated value as other functions do.]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/neon_helper.c |  139 ++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 127 insertions(+), 12 deletions(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index dc09968..cc63636 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -558,9 +558,28 @@ uint64_t HELPER(neon_shl_s64)(uint64_t valop, uint64_t shiftop)
     }} while (0)
 NEON_VOP(rshl_s8, neon_s8, 4)
 NEON_VOP(rshl_s16, neon_s16, 2)
-NEON_VOP(rshl_s32, neon_s32, 1)
 #undef NEON_FN
 
+/* The addition of the rounding constant may overflow, so we use an
+ * intermediate 64 bits accumulator.  */
+uint32_t HELPER(neon_rshl_s32)(uint32_t valop, uint32_t shiftop)
+{
+    int32_t dest;
+    int32_t val = (int32_t)valop;
+    int8_t shift = (int8_t)shiftop;
+    if ((shift >= 32) || (shift <= -32)) {
+        dest = 0;
+    } else if (shift < 0) {
+        int64_t big_dest = ((int64_t)val + (1 << (-1 - shift)));
+        dest = big_dest >> -shift;
+    } else {
+        dest = val << shift;
+    }
+    return dest;
+}
+
+/* Handling addition overflow with 64 bits inputs values is more
+ * tricky than with 32 bits values.  */
 uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
 {
     int8_t shift = (int8_t)shiftop;
@@ -574,7 +593,16 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
         val++;
         val >>= 1;
     } else if (shift < 0) {
-        val = (val + ((int64_t)1 << (-1 - shift))) >> -shift;
+        val >>= (-shift - 1);
+        if (val == INT64_MAX) {
+            /* In this case, it means that the rounding constant is 1,
+             * and the addition would overflow. Return the actual
+             * result directly.  */
+            val = 0x4000000000000000LL;
+        } else {
+            val++;
+            val >>= 1;
+        }
     } else {
         val <<= shift;
     }
@@ -596,9 +624,29 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
     }} while (0)
 NEON_VOP(rshl_u8, neon_u8, 4)
 NEON_VOP(rshl_u16, neon_u16, 2)
-NEON_VOP(rshl_u32, neon_u32, 1)
 #undef NEON_FN
 
+/* The addition of the rounding constant may overflow, so we use an
+ * intermediate 64 bits accumulator.  */
+uint32_t HELPER(neon_rshl_u32)(uint32_t val, uint32_t shiftop)
+{
+    uint32_t dest;
+    int8_t shift = (int8_t)shiftop;
+    if (shift >= 32 || shift < -32) {
+        dest = 0;
+    } else if (shift == -32) {
+        dest = val >> 31;
+    } else if (shift < 0) {
+        uint64_t big_dest = ((uint64_t)val + (1 << (-1 - shift)));
+        dest = big_dest >> -shift;
+    } else {
+        dest = val << shift;
+    }
+    return dest;
+}
+
+/* Handling addition overflow with 64 bits inputs values is more
+ * tricky than with 32 bits values.  */
 uint64_t HELPER(neon_rshl_u64)(uint64_t val, uint64_t shiftop)
 {
     int8_t shift = (uint8_t)shiftop;
@@ -607,9 +655,17 @@ uint64_t HELPER(neon_rshl_u64)(uint64_t val, uint64_t shiftop)
     } else if (shift == -64) {
         /* Rounding a 1-bit result just preserves that bit.  */
         val >>= 63;
-    } if (shift < 0) {
-        val = (val + ((uint64_t)1 << (-1 - shift))) >> -shift;
-        val >>= -shift;
+    } else if (shift < 0) {
+        val >>= (-shift - 1);
+        if (val == UINT64_MAX) {
+            /* In this case, it means that the rounding constant is 1,
+             * and the addition would overflow. Return the actual
+             * result directly.  */
+            val = 0x8000000000000000ULL;
+        } else {
+            val++;
+            val >>= 1;
+        }
     } else {
         val <<= shift;
     }
@@ -784,14 +840,43 @@ uint64_t HELPER(neon_qshlu_s64)(CPUState *env, uint64_t valop, uint64_t shiftop)
     }} while (0)
 NEON_VOP_ENV(qrshl_u8, neon_u8, 4)
 NEON_VOP_ENV(qrshl_u16, neon_u16, 2)
-NEON_VOP_ENV(qrshl_u32, neon_u32, 1)
 #undef NEON_FN
 
+/* The addition of the rounding constant may overflow, so we use an
+ * intermediate 64 bits accumulator.  */
+uint32_t HELPER(neon_qrshl_u32)(CPUState *env, uint32_t val, uint32_t shiftop)
+{
+    uint32_t dest;
+    int8_t shift = (int8_t)shiftop;
+    if (shift < 0) {
+        uint64_t big_dest = ((uint64_t)val + (1 << (-1 - shift)));
+        dest = big_dest >> -shift;
+    } else {
+        dest = val << shift;
+        if ((dest >> shift) != val) {
+            SET_QC();
+            dest = ~0;
+        }
+    }
+    return dest;
+}
+
+/* Handling addition overflow with 64 bits inputs values is more
+ * tricky than with 32 bits values.  */
 uint64_t HELPER(neon_qrshl_u64)(CPUState *env, uint64_t val, uint64_t shiftop)
 {
     int8_t shift = (int8_t)shiftop;
     if (shift < 0) {
-        val = (val + (1 << (-1 - shift))) >> -shift;
+        val >>= (-shift - 1);
+        if (val == UINT64_MAX) {
+            /* In this case, it means that the rounding constant is 1,
+             * and the addition would overflow. Return the actual
+             * result directly.  */
+            val = 0x8000000000000000ULL;
+        } else {
+            val++;
+            val >>= 1;
+        }
     } else { \
         uint64_t tmp = val;
         val <<= shift;
@@ -817,22 +902,52 @@ uint64_t HELPER(neon_qrshl_u64)(CPUState *env, uint64_t val, uint64_t shiftop)
     }} while (0)
 NEON_VOP_ENV(qrshl_s8, neon_s8, 4)
 NEON_VOP_ENV(qrshl_s16, neon_s16, 2)
-NEON_VOP_ENV(qrshl_s32, neon_s32, 1)
 #undef NEON_FN
 
+/* The addition of the rounding constant may overflow, so we use an
+ * intermediate 64 bits accumulator.  */
+uint32_t HELPER(neon_qrshl_s32)(CPUState *env, uint32_t valop, uint32_t shiftop)
+{
+    int32_t dest;
+    int32_t val = (int32_t)valop;
+    int8_t shift = (int8_t)shiftop;
+    if (shift < 0) {
+        int64_t big_dest = ((int64_t)val + (1 << (-1 - shift)));
+        dest = big_dest >> -shift;
+    } else {
+        dest = val << shift;
+        if ((dest >> shift) != val) {
+            SET_QC();
+            dest = (val >> 31) ^ ~SIGNBIT;
+        }
+    }
+    return dest;
+}
+
+/* Handling addition overflow with 64 bits inputs values is more
+ * tricky than with 32 bits values.  */
 uint64_t HELPER(neon_qrshl_s64)(CPUState *env, uint64_t valop, uint64_t shiftop)
 {
     int8_t shift = (uint8_t)shiftop;
     int64_t val = valop;
 
     if (shift < 0) {
-        val = (val + (1 << (-1 - shift))) >> -shift;
+        val >>= (-shift - 1);
+        if (val == INT64_MAX) {
+            /* In this case, it means that the rounding constant is 1,
+             * and the addition would overflow. Return the actual
+             * result directly.  */
+            val = 0x4000000000000000ULL;
+        } else {
+            val++;
+            val >>= 1;
+        }
     } else {
-        int64_t tmp = val;;
+        int64_t tmp = val;
         val <<= shift;
         if ((val >> shift) != tmp) {
             SET_QC();
-            val = tmp >> 31;
+            val = (tmp >> 63) ^ ~SIGNBIT64;
         }
     }
     return val;
-- 
1.7.1

[Qemu-devel] [PATCH 02/10] target-arm: Fix signed VRSHL by large shift counts

[Peter Maydell]

Correctly handle VRSHL of signed values by a shift count of the
width of the data type or larger, which must be special-cased in the
rshl_s* helper functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/neon_helper.c |   17 +++--------------
 1 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index cc63636..f692640 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -543,14 +543,9 @@ uint64_t HELPER(neon_shl_s64)(uint64_t valop, uint64_t shiftop)
 #define NEON_FN(dest, src1, src2) do { \
     int8_t tmp; \
     tmp = (int8_t)src2; \
-    if (tmp >= (ssize_t)sizeof(src1) * 8) { \
+    if ((tmp >= (ssize_t)sizeof(src1) * 8) \
+        || (tmp <= -(ssize_t)sizeof(src1) * 8)) { \
         dest = 0; \
-    } else if (tmp < -(ssize_t)sizeof(src1) * 8) { \
-        dest = src1 >> (sizeof(src1) * 8 - 1); \
-    } else if (tmp == -(ssize_t)sizeof(src1) * 8) { \
-        dest = src1 >> (tmp - 1); \
-        dest++; \
-        dest >>= 1; \
     } else if (tmp < 0) { \
         dest = (src1 + (1 << (-1 - tmp))) >> -tmp; \
     } else { \
@@ -584,14 +579,8 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
 {
     int8_t shift = (int8_t)shiftop;
     int64_t val = valop;
-    if (shift >= 64) {
+    if ((shift >= 64) || (shift <= -64)) {
         val = 0;
-    } else if (shift < -64) {
-        val >>= 63;
-    } else if (shift == -63) {
-        val >>= 63;
-        val++;
-        val >>= 1;
     } else if (shift < 0) {
         val >>= (-shift - 1);
         if (val == INT64_MAX) {
-- 
1.7.1

[Qemu-devel] [PATCH 03/10] target-arm: Fix unsigned VRSHL.s8 and .s16 right shifts by type width

[Peter Maydell]

From: Christophe Lyon <christophe.lyon@st.com>

Fix handling of unsigned VRSHL.s8 and .s16 right shifts by the type
width.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/neon_helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index f692640..2930b5e 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -605,7 +605,7 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
         tmp < -(ssize_t)sizeof(src1) * 8) { \
         dest = 0; \
     } else if (tmp == -(ssize_t)sizeof(src1) * 8) { \
-        dest = src1 >> (tmp - 1); \
+        dest = src1 >> (-tmp - 1); \
     } else if (tmp < 0) { \
         dest = (src1 + (1 << (-1 - tmp))) >> -tmp; \
     } else { \
-- 
1.7.1

[Qemu-devel] [PATCH 04/10] target-arm: fix unsigned 64 bit right shifts.

[Peter Maydell]

From: Christophe Lyon <christophe.lyon@st.com>

Fix range of shift amounts which always give 0 as result.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/neon_helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 2930b5e..a8885fa 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -639,7 +639,7 @@ uint32_t HELPER(neon_rshl_u32)(uint32_t val, uint32_t shiftop)
 uint64_t HELPER(neon_rshl_u64)(uint64_t val, uint64_t shiftop)
 {
     int8_t shift = (uint8_t)shiftop;
-    if (shift >= 64 || shift < 64) {
+    if (shift >= 64 || shift < -64) {
         val = 0;
     } else if (shift == -64) {
         /* Rounding a 1-bit result just preserves that bit.  */
-- 
1.7.1

[Qemu-devel] [PATCH 05/10] target-arm: Fix saturated values for Neon right shifts

[Peter Maydell]

Fix value returned by signed 8 and 16 bit qrshl helpers
when the result has saturated.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/neon_helper.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index a8885fa..7859f0b 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -886,7 +886,10 @@ uint64_t HELPER(neon_qrshl_u64)(CPUState *env, uint64_t val, uint64_t shiftop)
         dest = src1 << tmp; \
         if ((dest >> tmp) != src1) { \
             SET_QC(); \
-            dest = src1 >> 31; \
+            dest = (uint32_t)(1 << (sizeof(src1) * 8 - 1)); \
+            if (src1 > 0) { \
+                dest--; \
+            } \
         } \
     }} while (0)
 NEON_VOP_ENV(qrshl_s8, neon_s8, 4)
-- 
1.7.1

[Qemu-devel] [PATCH 06/10] target-arm: fix Neon VQSHRN and VSHRN.

[Peter Maydell]

From: Christophe Lyon <christophe.lyon@st.com>

Call the normal shift helpers instead of the rounding ones.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index 362d1d0..bd26b1b 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4095,8 +4095,8 @@ static inline void gen_neon_shift_narrow(int size, TCGv var, TCGv shift,
     } else {
         if (u) {
             switch (size) {
-            case 1: gen_helper_neon_rshl_u16(var, var, shift); break;
-            case 2: gen_helper_neon_rshl_u32(var, var, shift); break;
+            case 1: gen_helper_neon_shl_u16(var, var, shift); break;
+            case 2: gen_helper_neon_shl_u32(var, var, shift); break;
             default: abort();
             }
         } else {
-- 
1.7.1

[Qemu-devel] [PATCH 07/10] target-arm: fix decoding of Neon 64 bit shifts.

[Peter Maydell]

From: Christophe Lyon <christophe.lyon@st.com>

Fix decoding of 64 bits variants of VSHRN, VRSHRN, VQSHRN, VQSHRUN,
VQRSHRN, VQRSHRUN, taking into account whether inputs are unsigned
or not.

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   45 ++++++++++++++++++++++++++++++---------------
 1 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index bd26b1b..a02b20f 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4815,6 +4815,8 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
             } else if (op < 10) {
                 /* Shift by immediate and narrow:
                    VSHRN, VRSHRN, VQSHRN, VQRSHRN.  */
+                int input_unsigned = (op == 8) ? !u : u;
+
                 shift = shift - (1 << (size + 3));
                 size++;
                 switch (size) {
@@ -4841,33 +4843,46 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                     if (size == 3) {
                         neon_load_reg64(cpu_V0, rm + pass);
                         if (q) {
-                          if (u)
-                            gen_helper_neon_rshl_u64(cpu_V0, cpu_V0, tmp64);
-                          else
-                            gen_helper_neon_rshl_s64(cpu_V0, cpu_V0, tmp64);
+                            if (input_unsigned) {
+                                gen_helper_neon_rshl_u64(cpu_V0, cpu_V0,
+                                                         tmp64);
+                            } else {
+                                gen_helper_neon_rshl_s64(cpu_V0, cpu_V0,
+                                                         tmp64);
+                            }
                         } else {
-                          if (u)
-                            gen_helper_neon_shl_u64(cpu_V0, cpu_V0, tmp64);
-                          else
-                            gen_helper_neon_shl_s64(cpu_V0, cpu_V0, tmp64);
+                            if (input_unsigned) {
+                                gen_helper_neon_shl_u64(cpu_V0, cpu_V0,
+                                                        tmp64);
+                            } else {
+                                gen_helper_neon_shl_s64(cpu_V0, cpu_V0,
+                                                        tmp64);
+                            }
                         }
                     } else {
                         tmp = neon_load_reg(rm + pass, 0);
-                        gen_neon_shift_narrow(size, tmp, tmp2, q, u);
+                        gen_neon_shift_narrow(size, tmp, tmp2, q,
+                                              input_unsigned);
                         tmp3 = neon_load_reg(rm + pass, 1);
-                        gen_neon_shift_narrow(size, tmp3, tmp2, q, u);
+                        gen_neon_shift_narrow(size, tmp3, tmp2, q,
+                                              input_unsigned);
                         tcg_gen_concat_i32_i64(cpu_V0, tmp, tmp3);
                         dead_tmp(tmp);
                         dead_tmp(tmp3);
                     }
                     tmp = new_tmp();
-                    if (op == 8 && !u) {
-                        gen_neon_narrow(size - 1, tmp, cpu_V0);
+                    if (op == 8) {
+                        if (u) { /* VQSHRUN / VQRSHRUN */
+                            gen_neon_unarrow_sats(size - 1, tmp, cpu_V0);
+                        } else { /* VSHRN / VRSHRN */
+                            gen_neon_narrow(size - 1, tmp, cpu_V0);
+                        }
                     } else {
-                        if (op == 8)
-                            gen_neon_narrow_sats(size - 1, tmp, cpu_V0);
-                        else
+                        if (u) { /* VQSHRN / VQRSHRN */
                             gen_neon_narrow_satu(size - 1, tmp, cpu_V0);
+                        } else { /* VQSHRN / VQRSHRN */
+                            gen_neon_narrow_sats(size - 1, tmp, cpu_V0);
+                        }
                     }
                     neon_store_reg(rd, pass, tmp);
                 } /* for pass */
-- 
1.7.1

[Qemu-devel] [PATCH 08/10] target-arm: Fix signed VQRSHL by large shift counts

[Peter Maydell]

Handle the case of signed VQRSHL by a shift count of the width of the
data type or larger, which must be special cased in the qrshl_s*
helper functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/neon_helper.c |   34 +++++++++++++++++++++++++++++++---
 1 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 7859f0b..f8f6db6 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -880,7 +880,19 @@ uint64_t HELPER(neon_qrshl_u64)(CPUState *env, uint64_t val, uint64_t shiftop)
 #define NEON_FN(dest, src1, src2) do { \
     int8_t tmp; \
     tmp = (int8_t)src2; \
-    if (tmp < 0) { \
+    if (tmp >= (ssize_t)sizeof(src1) * 8) { \
+        if (src1) { \
+            SET_QC(); \
+            dest = (1 << (sizeof(src1) * 8 - 1)); \
+            if (src1 > 0) { \
+                dest--; \
+            } \
+        } else { \
+            dest = 0; \
+        } \
+    } else if (tmp <= -(ssize_t)sizeof(src1) * 8) { \
+        dest = 0; \
+    } else if (tmp < 0) { \
         dest = (src1 + (1 << (-1 - tmp))) >> -tmp; \
     } else { \
         dest = src1 << tmp; \
@@ -903,7 +915,16 @@ uint32_t HELPER(neon_qrshl_s32)(CPUState *env, uint32_t valop, uint32_t shiftop)
     int32_t dest;
     int32_t val = (int32_t)valop;
     int8_t shift = (int8_t)shiftop;
-    if (shift < 0) {
+    if (shift >= 32) {
+        if (val) {
+            SET_QC();
+            dest = (val >> 31) ^ ~SIGNBIT;
+        } else {
+            dest = 0;
+        }
+    } else if (shift <= -32) {
+        dest = 0;
+    } else if (shift < 0) {
         int64_t big_dest = ((int64_t)val + (1 << (-1 - shift)));
         dest = big_dest >> -shift;
     } else {
@@ -923,7 +944,14 @@ uint64_t HELPER(neon_qrshl_s64)(CPUState *env, uint64_t valop, uint64_t shiftop)
     int8_t shift = (uint8_t)shiftop;
     int64_t val = valop;
 
-    if (shift < 0) {
+    if (shift >= 64) {
+        if (val) {
+            SET_QC();
+            val = (val >> 63) ^ ~SIGNBIT64;
+        }
+    } else if (shift <= -64) {
+        val = 0;
+    } else if (shift < 0) {
         val >>= (-shift - 1);
         if (val == INT64_MAX) {
             /* In this case, it means that the rounding constant is 1,
-- 
1.7.1

[Qemu-devel] [PATCH 09/10] target-arm: Fix unsigned VQRSHL by large shift counts

[Peter Maydell]

Correctly handle VQRSHL of unsigned values by a shift count of the
width of the data type or larger, which must be special-cased in the
qrshl_u* helper functions.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/neon_helper.c |   37 ++++++++++++++++++++++++++++++++++---
 1 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index f8f6db6..c813fa0 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -818,7 +818,18 @@ uint64_t HELPER(neon_qshlu_s64)(CPUState *env, uint64_t valop, uint64_t shiftop)
 #define NEON_FN(dest, src1, src2) do { \
     int8_t tmp; \
     tmp = (int8_t)src2; \
-    if (tmp < 0) { \
+    if (tmp >= (ssize_t)sizeof(src1) * 8) { \
+        if (src1) { \
+            SET_QC(); \
+            dest = ~0; \
+        } else { \
+            dest = 0; \
+        } \
+    } else if (tmp < -(ssize_t)sizeof(src1) * 8) { \
+        dest = 0; \
+    } else if (tmp == -(ssize_t)sizeof(src1) * 8) { \
+        dest = src1 >> (sizeof(src1) * 8 - 1); \
+    } else if (tmp < 0) { \
         dest = (src1 + (1 << (-1 - tmp))) >> -tmp; \
     } else { \
         dest = src1 << tmp; \
@@ -837,7 +848,18 @@ uint32_t HELPER(neon_qrshl_u32)(CPUState *env, uint32_t val, uint32_t shiftop)
 {
     uint32_t dest;
     int8_t shift = (int8_t)shiftop;
-    if (shift < 0) {
+    if (shift >= 32) {
+        if (val) {
+            SET_QC();
+            dest = ~0;
+        } else {
+            dest = 0;
+        }
+    } else if (shift < -32) {
+        dest = 0;
+    } else if (shift == -32) {
+        dest = val >> 31;
+    } else if (shift < 0) {
         uint64_t big_dest = ((uint64_t)val + (1 << (-1 - shift)));
         dest = big_dest >> -shift;
     } else {
@@ -855,7 +877,16 @@ uint32_t HELPER(neon_qrshl_u32)(CPUState *env, uint32_t val, uint32_t shiftop)
 uint64_t HELPER(neon_qrshl_u64)(CPUState *env, uint64_t val, uint64_t shiftop)
 {
     int8_t shift = (int8_t)shiftop;
-    if (shift < 0) {
+    if (shift >= 64) {
+        if (val) {
+            SET_QC();
+            val = ~0;
+        }
+    } else if (shift < -64) {
+        val = 0;
+    } else if (shift == -64) {
+        val >>= 63;
+    } else if (shift < 0) {
         val >>= (-shift - 1);
         if (val == UINT64_MAX) {
             /* In this case, it means that the rounding constant is 1,
-- 
1.7.1

[Qemu-devel] [PATCH 10/10] target-arm: Fix shift by immediate and narrow where src, dest overlap

[Peter Maydell]

For Neon shifts by immediate and narrow, correctly handle the case
where the source registers and the destination registers overlap
(the second pass should use the original register contents, not the
results of the first pass).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate.c |   38 +++++++++++++++++++++++++++-----------
 1 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/target-arm/translate.c b/target-arm/translate.c
index a02b20f..4d5d305 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4839,31 +4839,47 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
                     abort();
                 }
 
+                if (size == 3) {
+                    neon_load_reg64(cpu_V0, rm);
+                    neon_load_reg64(cpu_V1, rm + 1);
+                } else {
+                    tmp4 = neon_load_reg(rm + 1, 0);
+                    tmp5 = neon_load_reg(rm + 1, 1);
+                }
                 for (pass = 0; pass < 2; pass++) {
                     if (size == 3) {
-                        neon_load_reg64(cpu_V0, rm + pass);
+                        TCGv_i64 in;
+                        if (pass == 0) {
+                            in = cpu_V0;
+                        } else {
+                            in = cpu_V1;
+                        }
                         if (q) {
                             if (input_unsigned) {
-                                gen_helper_neon_rshl_u64(cpu_V0, cpu_V0,
-                                                         tmp64);
+                                gen_helper_neon_rshl_u64(cpu_V0, in, tmp64);
                             } else {
-                                gen_helper_neon_rshl_s64(cpu_V0, cpu_V0,
-                                                         tmp64);
+                                gen_helper_neon_rshl_s64(cpu_V0, in, tmp64);
                             }
                         } else {
                             if (input_unsigned) {
-                                gen_helper_neon_shl_u64(cpu_V0, cpu_V0,
-                                                        tmp64);
+                                gen_helper_neon_shl_u64(cpu_V0, in, tmp64);
                             } else {
-                                gen_helper_neon_shl_s64(cpu_V0, cpu_V0,
-                                                        tmp64);
+                                gen_helper_neon_shl_s64(cpu_V0, in, tmp64);
                             }
                         }
                     } else {
-                        tmp = neon_load_reg(rm + pass, 0);
+                        if (pass == 0) {
+                            tmp = neon_load_reg(rm, 0);
+                        } else {
+                            tmp = tmp4;
+                        }
                         gen_neon_shift_narrow(size, tmp, tmp2, q,
                                               input_unsigned);
-                        tmp3 = neon_load_reg(rm + pass, 1);
+                        if (pass == 0) {
+                            tmp3 = neon_load_reg(rm, 1);
+                        } else {
+                            tmp3 = tmp5;
+                        }
                         gen_neon_shift_narrow(size, tmp3, tmp2, q,
                                               input_unsigned);
                         tcg_gen_concat_i32_i64(cpu_V0, tmp, tmp3);
-- 
1.7.1

[Qemu-devel] Re: [PATCH 00/10] Fix Neon shift instructions

[Christophe Lyon]

On 15.02.2011 14:44, Peter Maydell wrote:
> This patch series fixes bugs in the Neon shift instructions
> VRSHR, VRSRA, VQRSHRN, VQRSHRUN, VRSHRN, VQSHRN, VSHRN, VQSHRUN,
> VRSHL, and VQRSHL. It is based on the v3 patchset Christophe
> sent recently, with some fixes for minor nits in those patches,
> plus some patches from me which fix problems with shifts by
> large shift counts and an issue with overlapping source and
> destination registers.
> 
> With this patchset qemu passes random instruction sequence
> testing for all these instruction patterns.
> 

FWIW, I confirm that with this patchset, qemu passes all my tests on shift instruction.

Peter, thank you for adapting my patches and transforming my patch #4 into your patch #5.

Christophe.

Re: [Qemu-devel] [PATCH 10/10] target-arm: Fix shift by immediate and narrow where src, dest overlap

[Aurelien Jarno]

On Tue, Feb 15, 2011 at 01:44:50PM +0000, Peter Maydell wrote:
> For Neon shifts by immediate and narrow, correctly handle the case
> where the source registers and the destination registers overlap
> (the second pass should use the original register contents, not the
> results of the first pass).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/translate.c |   38 +++++++++++++++++++++++++++-----------
>  1 files changed, 27 insertions(+), 11 deletions(-)

That looks correct, but it makes GCC (tested 4.3 to 4.6) complaining:

| cc1: warnings being treated as errors
| qemu/target-arm/translate.c: In function ‘disas_neon_data_insn’:
| qemu/target-arm/translate.c:4185: error: ‘tmp4’ may be used uninitialized in this function
| qemu/target-arm/translate.c:4185: error: ‘tmp5’ may be used uninitialized in this function

For a quick look, it seems to be a GCC issue, but we have no other 
choices than workarouding it.

> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index a02b20f..4d5d305 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -4839,31 +4839,47 @@ static int disas_neon_data_insn(CPUState * env, DisasContext *s, uint32_t insn)
>                      abort();
>                  }
>  
> +                if (size == 3) {
> +                    neon_load_reg64(cpu_V0, rm);
> +                    neon_load_reg64(cpu_V1, rm + 1);
> +                } else {
> +                    tmp4 = neon_load_reg(rm + 1, 0);
> +                    tmp5 = neon_load_reg(rm + 1, 1);
> +                }
>                  for (pass = 0; pass < 2; pass++) {
>                      if (size == 3) {
> -                        neon_load_reg64(cpu_V0, rm + pass);
> +                        TCGv_i64 in;
> +                        if (pass == 0) {
> +                            in = cpu_V0;
> +                        } else {
> +                            in = cpu_V1;
> +                        }
>                          if (q) {
>                              if (input_unsigned) {
> -                                gen_helper_neon_rshl_u64(cpu_V0, cpu_V0,
> -                                                         tmp64);
> +                                gen_helper_neon_rshl_u64(cpu_V0, in, tmp64);
>                              } else {
> -                                gen_helper_neon_rshl_s64(cpu_V0, cpu_V0,
> -                                                         tmp64);
> +                                gen_helper_neon_rshl_s64(cpu_V0, in, tmp64);
>                              }
>                          } else {
>                              if (input_unsigned) {
> -                                gen_helper_neon_shl_u64(cpu_V0, cpu_V0,
> -                                                        tmp64);
> +                                gen_helper_neon_shl_u64(cpu_V0, in, tmp64);
>                              } else {
> -                                gen_helper_neon_shl_s64(cpu_V0, cpu_V0,
> -                                                        tmp64);
> +                                gen_helper_neon_shl_s64(cpu_V0, in, tmp64);
>                              }
>                          }
>                      } else {
> -                        tmp = neon_load_reg(rm + pass, 0);
> +                        if (pass == 0) {
> +                            tmp = neon_load_reg(rm, 0);
> +                        } else {
> +                            tmp = tmp4;
> +                        }
>                          gen_neon_shift_narrow(size, tmp, tmp2, q,
>                                                input_unsigned);
> -                        tmp3 = neon_load_reg(rm + pass, 1);
> +                        if (pass == 0) {
> +                            tmp3 = neon_load_reg(rm, 1);
> +                        } else {
> +                            tmp3 = tmp5;
> +                        }
>                          gen_neon_shift_narrow(size, tmp3, tmp2, q,
>                                                input_unsigned);
>                          tcg_gen_concat_i32_i64(cpu_V0, tmp, tmp3);
> -- 
> 1.7.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 00/10] Fix Neon shift instructions

[Aurelien Jarno]

On Tue, Feb 15, 2011 at 01:44:40PM +0000, Peter Maydell wrote:
> This patch series fixes bugs in the Neon shift instructions
> VRSHR, VRSRA, VQRSHRN, VQRSHRUN, VRSHRN, VQSHRN, VSHRN, VQSHRUN,
> VRSHL, and VQRSHL. It is based on the v3 patchset Christophe
> sent recently, with some fixes for minor nits in those patches,
> plus some patches from me which fix problems with shifts by
> large shift counts and an issue with overlapping source and
> destination registers.
> 
> With this patchset qemu passes random instruction sequence
> testing for all these instruction patterns.
> 
> Christophe Lyon (5):
>   target-arm: Fix rounding constant addition for Neon shifts
>   target-arm: Fix unsigned VRSHL.s8 and .s16 right shifts by type width
>   target-arm: fix unsigned 64 bit right shifts.
>   target-arm: fix Neon VQSHRN and VSHRN.
>   target-arm: fix decoding of Neon 64 bit shifts.
> 
> Peter Maydell (5):
>   target-arm: Fix signed VRSHL by large shift counts
>   target-arm: Fix saturated values for Neon right shifts
>   target-arm: Fix signed VQRSHL by large shift counts
>   target-arm: Fix unsigned VQRSHL by large shift counts
>   target-arm: Fix shift by immediate and narrow where src,dest overlap
> 
>  target-arm/neon_helper.c |  232 +++++++++++++++++++++++++++++++++++++++-------
>  target-arm/translate.c   |   71 ++++++++++----
>  2 files changed, 250 insertions(+), 53 deletions(-)
> 

Thanks, I applied patches 1 to 9. See my comment for the patch 10.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net