diff mbox

[2/6] target-arm: fix Neon right shifts with shift amount == input width.

Message ID 1297437062-6118-3-git-send-email-christophe.lyon@st.com
State New
Headers show

Commit Message

Christophe Lyon Feb. 11, 2011, 3:10 p.m. UTC 
From: Christophe Lyon <christophe.lyon@st.com>

Fix rshl helpers (s8, s16, s64, u8, u16)

Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
---
 target-arm/neon_helper.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Comments

Peter Maydell Feb. 14, 2011, 6:16 p.m. UTC | #1 
On 11 February 2011 15:10,  <christophe.lyon@st.com> wrote:
> From: Christophe Lyon <christophe.lyon@st.com>
>
> Fix rshl helpers (s8, s16, s64, u8, u16)
>
> Signed-off-by: Christophe Lyon <christophe.lyon@st.com>
> ---
>  target-arm/neon_helper.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
> index 3f1f3d4..1ac362f 100644
> --- a/target-arm/neon_helper.c
> +++ b/target-arm/neon_helper.c
> @@ -548,7 +548,7 @@ uint64_t HELPER(neon_shl_s64)(uint64_t valop, uint64_t shiftop)
>     } 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 = src1 >> (-tmp - 1); \
>         dest++; \
>         dest >>= 1; \

Again, these three lines have the same effect as dest = 0,
so we can fold into the previous if().

>     } else if (tmp < 0) { \
> @@ -594,7 +594,7 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
>         val = 0;
>     } else if (shift < -64) {
>         val >>= 63;

You didn't change this case, but it is the wrong answer:
should be 0.

> -    } else if (shift == -63) {
> +    } else if (shift == -64) {
>         val >>= 63;
>         val++;
>         val >>= 1;

Always results in 0.

-- PMM
Christophe Lyon Feb. 15, 2011, 1:47 p.m. UTC | #2 
>> -        dest = src1 >> (tmp - 1); \
>> +        dest = src1 >> (-tmp - 1); \
>>         dest++; \
>>         dest >>= 1; \
> 
> Again, these three lines have the same effect as dest = 0,
> so we can fold into the previous if().
> 
>>     } else if (tmp < 0) { \
>> @@ -594,7 +594,7 @@ uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
>>         val = 0;
>>     } else if (shift < -64) {
>>         val >>= 63;
> 
> You didn't change this case, but it is the wrong answer:
> should be 0.
> 
>> -    } else if (shift == -63) {
>> +    } else if (shift == -64) {
>>         val >>= 63;
>>         val++;
>>         val >>= 1;
> 
> Always results in 0.
> 


Oops sorry, in these 3 cases, I just fixed obvious typos but didn't question the actual code.
diff mbox

Patch

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 3f1f3d4..1ac362f 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -548,7 +548,7 @@  uint64_t HELPER(neon_shl_s64)(uint64_t valop, uint64_t shiftop)
     } 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 = src1 >> (-tmp - 1); \
         dest++; \
         dest >>= 1; \
     } else if (tmp < 0) { \
@@ -594,7 +594,7 @@  uint64_t HELPER(neon_rshl_s64)(uint64_t valop, uint64_t shiftop)
         val = 0;
     } else if (shift < -64) {
         val >>= 63;
-    } else if (shift == -63) {
+    } else if (shift == -64) {
         val >>= 63;
         val++;
         val >>= 1;
@@ -622,7 +622,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 { \