[Qemu-devel] [ARM] Problem in a NEON instruction

[Qemu-devel] [ARM] Problem in a NEON instruction

[Laurent Desnogues]

[-- Attachment #1: Type: text/plain, Size: 154 bytes --]

The rshl instruction is faulty.  Note I did not check the correctness
of that instruction in general, I only made a change that looked logical.


Laurent

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: arm-neon.patch --]
[-- Type: text/x-patch; name=arm-neon.patch, Size: 460 bytes --]

--- svn-ref/target-arm/neon_helper.c	2008-06-09 08:52:48.000000000 +0200
+++ svn/target-arm/neon_helper.c	2008-06-10 12:27:38.000000000 +0200
@@ -456,7 +456,7 @@
     if (tmp >= sizeof(src1) * 8) { \
         dest = 0; \
     } else if (tmp < -sizeof(src1) * 8) { \
-        dest >>= sizeof(src1) * 8 - 1; \
+        dest = src1 >> (sizeof(src1) * 8 - 1); \
     } else if (tmp == -sizeof(src1) * 8) { \
         dest = src1 >> (tmp - 1); \
         dest++; \

Re: [Qemu-devel] [ARM] Problem in a NEON instruction

[Aurelien Jarno]

On Tue, Jun 10, 2008 at 12:32:17PM +0200, Laurent Desnogues wrote:
> The rshl instruction is faulty.  Note I did not check the correctness
> of that instruction in general, I only made a change that looked logical.
> 
> 
> Laurent

> --- svn-ref/target-arm/neon_helper.c	2008-06-09 08:52:48.000000000 +0200
> +++ svn/target-arm/neon_helper.c	2008-06-10 12:27:38.000000000 +0200
> @@ -456,7 +456,7 @@
>      if (tmp >= sizeof(src1) * 8) { \
>          dest = 0; \
>      } else if (tmp < -sizeof(src1) * 8) { \
> -        dest >>= sizeof(src1) * 8 - 1; \
> +        dest = src1 >> (sizeof(src1) * 8 - 1); \
>      } else if (tmp == -sizeof(src1) * 8) { \
>          dest = src1 >> (tmp - 1); \
>          dest++; \

Your patch is indeed correct. It seems there is another mistake (at
least compared to the 64-bit version) in this instruction, please find
an updated patch below.

diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 4ee5658..35fbaf5 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -456,11 +456,11 @@ uint64_t HELPER(neon_shl_s64)(uint64_t valop, uint64_t shiftop)
     if (tmp >= sizeof(src1) * 8) { \
         dest = 0; \
     } else if (tmp < -sizeof(src1) * 8) { \
-        dest >>= sizeof(src1) * 8 - 1; \
+        dest = src1 >> (sizeof(src1) * 8 - 1); \
     } else if (tmp == -sizeof(src1) * 8) { \
         dest = src1 >> (tmp - 1); \
         dest++; \
-        src2 >>= 1; \
+        dest >>= 1; \
     } else if (tmp < 0) { \
         dest = (src1 + (1 << (-1 - tmp))) >> -tmp; \
     } else { \


-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [Qemu-devel] [ARM] Problem in a NEON instruction

[Laurent Desnogues]

On Tue, Oct 14, 2008 at 11:23 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
>
> Your patch is indeed correct. It seems there is another mistake (at
> least compared to the 64-bit version) in this instruction, please find
> an updated patch below.
>
> diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
> index 4ee5658..35fbaf5 100644
> --- a/target-arm/neon_helper.c
> +++ b/target-arm/neon_helper.c
> @@ -456,11 +456,11 @@ uint64_t HELPER(neon_shl_s64)(uint64_t valop, uint64_t shiftop)
>     if (tmp >= sizeof(src1) * 8) { \
>         dest = 0; \
>     } else if (tmp < -sizeof(src1) * 8) { \
> -        dest >>= sizeof(src1) * 8 - 1; \
> +        dest = src1 >> (sizeof(src1) * 8 - 1); \
>     } else if (tmp == -sizeof(src1) * 8) { \
>         dest = src1 >> (tmp - 1); \
>         dest++; \
> -        src2 >>= 1; \
> +        dest >>= 1; \
>     } else if (tmp < 0) { \
>         dest = (src1 + (1 << (-1 - tmp))) >> -tmp; \
>     } else { \

Your addition looks good to me.


Laurent

Re: [Qemu-devel] [ARM] Problem in a NEON instruction

[Laurent Desnogues]

On Tue, Oct 14, 2008 at 10:23 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Tue, Jun 10, 2008 at 12:32:17PM +0200, Laurent Desnogues wrote:
>> The rshl instruction is faulty.  Note I did not check the correctness
>> of that instruction in general, I only made a change that looked logical.
>>
>>
>> Laurent
>
>> --- svn-ref/target-arm/neon_helper.c  2008-06-09 08:52:48.000000000 +0200
>> +++ svn/target-arm/neon_helper.c      2008-06-10 12:27:38.000000000 +0200
>> @@ -456,7 +456,7 @@
>>      if (tmp >= sizeof(src1) * 8) { \
>>          dest = 0; \
>>      } else if (tmp < -sizeof(src1) * 8) { \
>> -        dest >>= sizeof(src1) * 8 - 1; \
>> +        dest = src1 >> (sizeof(src1) * 8 - 1); \
>>      } else if (tmp == -sizeof(src1) * 8) { \
>>          dest = src1 >> (tmp - 1); \
>>          dest++; \
>
> Your patch is indeed correct. It seems there is another mistake (at
> least compared to the 64-bit version) in this instruction, please find
> an updated patch below.
>
> diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
> index 4ee5658..35fbaf5 100644
> --- a/target-arm/neon_helper.c
> +++ b/target-arm/neon_helper.c
> @@ -456,11 +456,11 @@ uint64_t HELPER(neon_shl_s64)(uint64_t valop, uint64_t shiftop)
>     if (tmp >= sizeof(src1) * 8) { \
>         dest = 0; \
>     } else if (tmp < -sizeof(src1) * 8) { \
> -        dest >>= sizeof(src1) * 8 - 1; \
> +        dest = src1 >> (sizeof(src1) * 8 - 1); \
>     } else if (tmp == -sizeof(src1) * 8) { \
>         dest = src1 >> (tmp - 1); \
>         dest++; \
> -        src2 >>= 1; \
> +        dest >>= 1; \
>     } else if (tmp < 0) { \
>         dest = (src1 + (1 << (-1 - tmp))) >> -tmp; \
>     } else { \

Can someone consider that patch especially now that gcc 4
displays some well deserved warnings about the code :-)


Laurent