[PATCH][GCC][Arm] Rewrite arm testcase to use intrinsics

[PATCH][GCC][Arm] Rewrite arm testcase to use intrinsics

[Tamar Christina]

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

Hi All,

This test was added back when builtins were being used instead of ACLE
intrinsics.  The test as far as I can tell is really testing vcombine,
however some of these builtins no longer exist and causes an ICE.

This fixes the testcase by changing it to use neon intrinsics.

Regtested on arm-none-eabi and no issues.

Ok for trunk?

Thanks,
Tamar

gcc/testsuite/ChangeLog:

2019-01-17  Tamar Christina  <tamar.christina@arm.com>

	PR target/88850
	* gcc.target/arm/pr51968.c: Use neon intrinsics.

-- 

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

diff --git a/gcc/testsuite/gcc.target/arm/pr51968.c b/gcc/testsuite/gcc.target/arm/pr51968.c
index 99bdb961757bfa62aec5ef1426137425e57898b0..781470223db0d85214bced0b64fda68b4c43967f 100644
--- a/gcc/testsuite/gcc.target/arm/pr51968.c
+++ b/gcc/testsuite/gcc.target/arm/pr51968.c
@@ -1,14 +1,10 @@
 /* PR target/51968 */
 /* { dg-do compile } */
-/* { dg-options "-O2 -Wno-implicit-function-declaration -march=armv7-a -mfloat-abi=softfp -mfpu=neon" } */
+/* { dg-options "-O2 -march=armv7-a -mfloat-abi=softfp -mfpu=neon" } */
 /* { dg-require-effective-target arm_neon_ok } */
+#include <arm_neon.h>
 
-typedef __builtin_neon_qi int8x8_t __attribute__ ((__vector_size__ (8)));
-typedef __builtin_neon_uqi uint8x8_t __attribute__ ((__vector_size__ (8)));
-typedef __builtin_neon_qi int8x16_t __attribute__ ((__vector_size__ (16)));
-typedef __builtin_neon_hi int16x8_t __attribute__ ((__vector_size__ (16)));
-typedef __builtin_neon_si int32x4_t __attribute__ ((__vector_size__ (16)));
-struct T { int8x8_t val[2]; };
+struct T { int8x8x2_t val; };
 int y;
 
 void
@@ -17,16 +13,16 @@ foo (int8x8_t z, int8x8_t x, int16x8_t b, int8x8_t n)
   if (y)
     {
       struct T m;
-      __builtin_neon_vuzpv8qi (&m.val[0], z, x);
+      m.val = vuzp_s8 (z, x);
     }
   for (;;)
     {
       int8x16_t g;
       int8x8_t h, j, k;
       struct T m;
-      j = __builtin_neon_vqmovunv8hi (b);
-      g = __builtin_neon_vcombinev8qi (j, h);
-      k = __builtin_neon_vget_lowv16qi (g);
-      __builtin_neon_vuzpv8qi (&m.val[0], k, n);
+      j = vqmovn_s16 (b);
+      g = vcombine_s8 (j, h);
+      k = vget_low_s8 (g);
+      m.val = vuzp_s8 (k, n);
     }
 }

Re: [PATCH][GCC][Arm] Rewrite arm testcase to use intrinsics

[Ramana Radhakrishnan]

On 17/01/2019 15:02, Tamar Christina wrote:
> Hi All,
> 
> This test was added back when builtins were being used instead of ACLE
> intrinsics.  The test as far as I can tell is really testing vcombine,
> however some of these builtins no longer exist and causes an ICE.
> 
> This fixes the testcase by changing it to use neon intrinsics.
> 
> Regtested on arm-none-eabi and no issues.
> 
> Ok for trunk?
> 
> Thanks,
> Tamar
> 
> gcc/testsuite/ChangeLog:
> 
> 2019-01-17  Tamar Christina  <tamar.christina@arm.com>
> 
> 	PR target/88850
> 	* gcc.target/arm/pr51968.c: Use neon intrinsics.
> 

Ok.

Looks pretty obvious to me.

Ramana

Re: [PATCH][GCC][Arm] Rewrite arm testcase to use intrinsics

[Segher Boessenkool]

Hi!

On Thu, Jan 17, 2019 at 03:02:00PM +0000, Tamar Christina wrote:
> This test was added back when builtins were being used instead of ACLE
> intrinsics.  The test as far as I can tell is really testing vcombine,
> however some of these builtins no longer exist and causes an ICE.
> 
> This fixes the testcase by changing it to use neon intrinsics.

Shouldn't the ICE be fixed as well?  [ Sorry if you send a separate patch
for that and I missed it ].


Segher

RE: [PATCH][GCC][Arm] Rewrite arm testcase to use intrinsics

[Tamar Christina]

Hi Segher,

Yes, that's why the PR is still open 😊
The ICE can be reproduced with a much simpler testcase which is the one we use for repro in the PR, which is now a P1.

The reason for changing this testcase was that it was invalid code.

Regards,
Tamar

-----Original Message-----
From: Segher Boessenkool <segher@kernel.crashing.org> 
Sent: Sunday, January 20, 2019 3:48 PM
To: Tamar Christina <Tamar.Christina@arm.com>
Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com>; Richard Earnshaw <Richard.Earnshaw@arm.com>; nickc@redhat.com; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
Subject: Re: [PATCH][GCC][Arm] Rewrite arm testcase to use intrinsics

Hi!

On Thu, Jan 17, 2019 at 03:02:00PM +0000, Tamar Christina wrote:
> This test was added back when builtins were being used instead of ACLE 
> intrinsics.  The test as far as I can tell is really testing vcombine, 
> however some of these builtins no longer exist and causes an ICE.
> 
> This fixes the testcase by changing it to use neon intrinsics.

Shouldn't the ICE be fixed as well?  [ Sorry if you send a separate patch for that and I missed it ].


Segher

Re: [PATCH][GCC][Arm] Rewrite arm testcase to use intrinsics

[Ramana Radhakrishnan]

On 20/01/2019 15:48, Segher Boessenkool wrote:
> Hi!
> 
> On Thu, Jan 17, 2019 at 03:02:00PM +0000, Tamar Christina wrote:
>> This test was added back when builtins were being used instead of ACLE
>> intrinsics.  The test as far as I can tell is really testing vcombine,
>> however some of these builtins no longer exist and causes an ICE.
>>
>> This fixes the testcase by changing it to use neon intrinsics.

JFTR, I think this was a case when we were using builtins for
implementation of the ACLE intrinsics and the testcase was reduced to
remove the use of arm_neon.h . Thus the test needs to go back to using
the neon intrinsics directly if possible.

Also if there are other tests like this it would be a good small cleanup
to do.

> 
> Shouldn't the ICE be fixed as well?  [ Sorry if you send a separate patch
> for that and I missed it ].
> 

Indeed but that's a separate issue to this.

regards
Ramana
> 
> Segher
>