[12/12,RESEND] target-arm: fix neon shift helper functions
diff mbox

Message ID 3B23632B-8EC3-4CED-9E73-C1EC7142C9C3@nokia.com
State New
Headers show

Commit Message

Juha.Riihimaki@nokia.com Oct. 21, 2009, 11:01 a.m. UTC
Current code is broken at least on gcc 4.2, the result of a comparison
"-1 >= sizeof(type) * 8" results true and causes wrong code path to be
taken. The fix utilizes abs() function where applicable and otherwise
adds a test to ensure both arguments are positive before making the
aforementioned comparison.

Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
---
          dest = src1 >> (sizeof(src1) * 8 - 1); \
@@ -453,7 +453,7 @@ 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 >= sizeof(src1) * 8) { \
+    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
          dest = 0; \
      } else if (tmp < -sizeof(src1) * 8) { \
          dest = src1 >> (sizeof(src1) * 8 - 1); \
@@ -494,7 +494,7 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop,  
uint64_t shiftop)
  #define NEON_FN(dest, src1, src2) do { \
      int8_t tmp; \
      tmp = (int8_t)src2; \
-    if (tmp >= sizeof(src1) * 8 || tmp < -sizeof(src1) * 8) { \
+    if (abs(tmp) >= sizeof(src1) * 8) { \
          dest = 0; \
      } else if (tmp == -sizeof(src1) * 8) { \
          dest = src1 >> (tmp - 1); \
@@ -528,7 +528,7 @@ uint64_t HELPER(neon_rshl_u64)(uint64_t val,  
uint64_t shiftop)
  #define NEON_FN(dest, src1, src2) do { \
      int8_t tmp; \
      tmp = (int8_t)src2; \
-    if (tmp >= sizeof(src1) * 8) { \
+    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
          if (src1) { \
              SET_QC(); \
              dest = ~0; \
@@ -579,7 +579,7 @@ uint64_t HELPER(neon_qshl_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 >= sizeof(src1) * 8) { \
+    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
          if (src1) \
              SET_QC(); \
          dest = src1 >> 31; \

Comments

Laurent Desnogues Oct. 22, 2009, 7:57 a.m. UTC | #1
On Wed, Oct 21, 2009 at 1:01 PM,  <Juha.Riihimaki@nokia.com> wrote:
> Current code is broken at least on gcc 4.2, the result of a comparison
> "-1 >= sizeof(type) * 8" results true and causes wrong code path to be
> taken. The fix utilizes abs() function where applicable and otherwise
> adds a test to ensure both arguments are positive before making the
> aforementioned comparison.

Why don't you instead cast sizeof to int?


Laurent

> Signed-off-by: Juha Riihimäki <juha.riihimaki@nokia.com>
> ---
> diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
> index f32ecd6..0c95035 100644
> --- a/target-arm/neon_helper.c
> +++ b/target-arm/neon_helper.c
> @@ -392,7 +392,7 @@ NEON_VOP(abd_u32, neon_u32, 1)
>  #define NEON_FN(dest, src1, src2) do { \
>      int8_t tmp; \
>      tmp = (int8_t)src2; \
> -    if (tmp >= sizeof(src1) * 8 || tmp <= -sizeof(src1) * 8) { \
> +    if (abs(tmp) >= sizeof(src1) * 8) { \
>          dest = 0; \
>      } else if (tmp < 0) { \
>          dest = src1 >> -tmp; \
> @@ -420,7 +420,7 @@ uint64_t HELPER(neon_shl_u64)(uint64_t val,
> uint64_t shiftop)
>  #define NEON_FN(dest, src1, src2) do { \
>      int8_t tmp; \
>      tmp = (int8_t)src2; \
> -    if (tmp >= sizeof(src1) * 8) { \
> +    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
>          dest = 0; \
>      } else if (tmp <= -sizeof(src1) * 8) { \
>          dest = src1 >> (sizeof(src1) * 8 - 1); \
> @@ -453,7 +453,7 @@ 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 >= sizeof(src1) * 8) { \
> +    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
>          dest = 0; \
>      } else if (tmp < -sizeof(src1) * 8) { \
>          dest = src1 >> (sizeof(src1) * 8 - 1); \
> @@ -494,7 +494,7 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop,
> uint64_t shiftop)
>  #define NEON_FN(dest, src1, src2) do { \
>      int8_t tmp; \
>      tmp = (int8_t)src2; \
> -    if (tmp >= sizeof(src1) * 8 || tmp < -sizeof(src1) * 8) { \
> +    if (abs(tmp) >= sizeof(src1) * 8) { \
>          dest = 0; \
>      } else if (tmp == -sizeof(src1) * 8) { \
>          dest = src1 >> (tmp - 1); \
> @@ -528,7 +528,7 @@ uint64_t HELPER(neon_rshl_u64)(uint64_t val,
> uint64_t shiftop)
>  #define NEON_FN(dest, src1, src2) do { \
>      int8_t tmp; \
>      tmp = (int8_t)src2; \
> -    if (tmp >= sizeof(src1) * 8) { \
> +    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
>          if (src1) { \
>              SET_QC(); \
>              dest = ~0; \
> @@ -579,7 +579,7 @@ uint64_t HELPER(neon_qshl_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 >= sizeof(src1) * 8) { \
> +    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
>          if (src1) \
>              SET_QC(); \
>          dest = src1 >> 31; \
>
Juha.Riihimaki@nokia.com Oct. 22, 2009, 8:46 a.m. UTC | #2
On Oct 22, 2009, at 10:57, ext Laurent Desnogues wrote:

> On Wed, Oct 21, 2009 at 1:01 PM,  <Juha.Riihimaki@nokia.com> wrote:
>> Current code is broken at least on gcc 4.2, the result of a  
>> comparison
>> "-1 >= sizeof(type) * 8" results true and causes wrong code path to  
>> be
>> taken. The fix utilizes abs() function where applicable and otherwise
>> adds a test to ensure both arguments are positive before making the
>> aforementioned comparison.
>
> Why don't you instead cast sizeof to int?

No reason, I'll make the v2 patch series use cast instead. It seems to  
work just as well.


Juha

Patch
diff mbox

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index f32ecd6..0c95035 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -392,7 +392,7 @@  NEON_VOP(abd_u32, neon_u32, 1)
  #define NEON_FN(dest, src1, src2) do { \
      int8_t tmp; \
      tmp = (int8_t)src2; \
-    if (tmp >= sizeof(src1) * 8 || tmp <= -sizeof(src1) * 8) { \
+    if (abs(tmp) >= sizeof(src1) * 8) { \
          dest = 0; \
      } else if (tmp < 0) { \
          dest = src1 >> -tmp; \
@@ -420,7 +420,7 @@  uint64_t HELPER(neon_shl_u64)(uint64_t val,  
uint64_t shiftop)
  #define NEON_FN(dest, src1, src2) do { \
      int8_t tmp; \
      tmp = (int8_t)src2; \
-    if (tmp >= sizeof(src1) * 8) { \
+    if (tmp >= 0 && tmp >= sizeof(src1) * 8) { \
          dest = 0; \
      } else if (tmp <= -sizeof(src1) * 8) { \