Re: [PATCH 2/2][AArch64] Add missing support for poly64x1_t

[James Greenhalgh]

On Mon, Sep 19, 2016 at 03:22:39PM +0200, Christophe Lyon wrote:
> Hi,
> 
> 
> On 19 September 2016 at 10:39, Tamar Christina  
> wrote:
> >
> >
> > On 17/09/16 07:20, James Greenhalgh wrote:
> >
> >
> > Hi James,
> >
> >> The convention when fixing a bugzilla is to place a line in your ChangeLog
> >> which looks like:
> >>
> >> PR component/x
> >
> > Oh, I wasn't aware of this, thanks! I'll add it.
> >>
> >>  FAIL: gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c   -O0  (test
> >> for excess errors)
> >>  Excess errors:
> >>
> >> .../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:171:5:
> >> warning: implicit declaration of function 'vmov_n_p64'; did you mean
> >> 'vmov_n_u64'? [-Wimplicit-function
> >
> >
> > It looks like ARM is missing some of the poly64 functions. I can add these
> > since they don't seem to be too many.
> >
> I had opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71233 to track 
> missing
> poly64 intrinsics for the ARM target. Not sure why vmov_n_p64 isn't in the 
> list?

Now you mention it, I don't see vmov_n_p64 in the list of intrinsics at:

  
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0073a/IHI0073A_arm_neon_intrinsics_ref.pdf

So I wonder whether this patch would actually add some extra intrinsics
that we don't require?

Thanks,
James

Re: [PATCH 2/2][AArch64] Add missing support for poly64x1_t

[Christophe Lyon]

Hi,


On 19 September 2016 at 10:39, Tamar Christina  wrote:
>
>
> On 17/09/16 07:20, James Greenhalgh wrote:
>
>
> Hi James,
>
>> The convention when fixing a bugzilla is to place a line in your ChangeLog
>> which looks like:
>>
>> PR component/x
>
> Oh, I wasn't aware of this, thanks! I'll add it.
>>
>>  FAIL: gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c   -O0  (test
>> for excess errors)
>>  Excess errors:
>>
>> .../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:171:5:
>> warning: implicit declaration of function 'vmov_n_p64'; did you mean
>> 'vmov_n_u64'? [-Wimplicit-function
>
>
> It looks like ARM is missing some of the poly64 functions. I can add these
> since they don't seem to be too many.
>
I had opened https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71233 to track missing
poly64 intrinsics for the ARM target. Not sure why vmov_n_p64 isn't in the list?

> Regards,
> Tamar
>

Re: [PATCH 2/2][AArch64] Add missing support for poly64x1_t

[Tamar Christina]

On 17/09/16 07:20, James Greenhalgh wrote:


Hi James,


The convention when fixing a bugzilla is to place a line in your ChangeLog
which looks like:

PR component/x

Oh, I wasn't aware of this, thanks! I'll add it.

 FAIL: gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c   -O0  (test for 
excess errors)
 Excess errors:
 .../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:171:5: 
warning: implicit declaration of function 'vmov_n_p64'; did you mean 
'vmov_n_u64'? [-Wimplicit-function


It looks like ARM is missing some of the poly64 functions. I can add 
these since they don't seem to be too many.


Regards,
Tamar

Re: [PATCH 2/2][AArch64] Add missing support for poly64x1_t

[James Greenhalgh]

On Tue, Sep 13, 2016 at 01:54:15PM +0100, Tamar Christina wrote:
> Hi all,
> 
> This patch adds the following NEON intrinsics to the ARM Aarch64 GCC
> (and fixes https://gcc.gnu.org/bugzilla/show_bug.cgi?id=72758):
> 



> Added new tests for these and ran regression tests on aarch64-none-linux-gnu.
> 
> Ok for trunk?

Hi Tamar,

Thanks for the patch! If I'm not mistaken, this also fixes PR target/58693.

The convention when fixing a bugzilla is to place a line in your ChangeLog
which looks like:

PR component/x

This then gets picked up by a little robot which scrapes the commits, and
updates bugzilla with a link to the commit that fixes the bug. See
gcc/ChangeLog for examples.

This patch looks OK to me functionally, but I'd appreciate a second set
of eyes on the testsuite changes. I've CC'ed Christophe Lyon who wrote
the testsuite. Do remember that these testcases are shared with the ARM
port, so it is appropriate to test an ARM build against your changes. In
this case, I see your patch introduces some errors when testing the ARM
target. Presumably the check against __ARM_FEATURE_CRYPTO is not strong
enough to ensure that the poly64_t types are available.

FAIL: gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c   -O0  (test for 
excess errors)
Excess errors:
.../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:171:5: 
warning: implicit declaration of function 'vmov_n_p64'; did you mean 
'vmov_n_u64'? [-Wimplicit-function-declaration]
.../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:171:5: 
warning: implicit declaration of function 'vmovq_n_p64'; did you mean 
'vmovq_n_u64'? [-Wimplicit-function-declaration]
.../gcc/testsuite/gcc.target/aarch64/advsimd-intrinsics/vdup-vmov.c:170:30: 
error: incompatible types when assigning to type 'poly64x2_t {aka __vector(2) 
long long unsigned int}' from type 'int'

FAIL: gcc.target/aarch64/advsimd-intrinsics/vget_lane.c   -O0  (test for 
excess errors)
FAIL: gcc.target/aarch64/advsimd-intrinsics/vldX.c   -O0  (test for excess 
errors)
FAIL: gcc.target/aarch64/advsimd-intrinsics/vldX_lane.c   -O0  (test for 
excess errors)
FAIL: gcc.target/aarch64/advsimd-intrinsics/vreinterpret_p128.c   -O0  
(test for excess errors)
FAIL: gcc.target/aarch64/advsimd-intrinsics/vreinterpret_p64.c   -O0  (test 
for excess errors)
FAIL: gcc.target/aarch64/advsimd-intrinsics/vstX_lane.c   -O0  (test for 
excess errors)

Could you take a look at this?

Thanks,
James