On 29/11/16 09:50, Tamar Christina wrote:
Hi All,
The new patch contains the proper types for the intrinsics that should be
returning uint64x1
and has the rest of the comments by Christophe in them.
Ok with an appropriate ChangeLog entry.
Thanks,
Kyrill
Kind Regards,
Tamar
________________________________________
From: Tamar Christina
Sent: Friday, November 25, 2016 4:01:30 PM
To: Christophe Lyon
Cc: GCC Patches; christophe.l...@st.com; Marcus Shawcroft; Richard Earnshaw;
James Greenhalgh; Kyrylo Tkachov; nd
Subject: RE: [AArch64][ARM][GCC][PATCHv2 3/3] Add tests for missing Poly64_t
intrinsics to GCC
>
A few comments about this new version:
* arm-neon-ref.h: why do you create
CHECK_RESULTS_NAMED_NO_FP16_NO_POLY64?
Can't you just add calls to CHECK_CRYPTO in the existing
CHECK_RESULTS_NAMED_NO_FP16?
Yes, that should be fine, I didn't used to have CHECK_CRYPTO before and when I
added it
I didn't remove the split. I'll do it now.
* p64_p128:
From what I can see ARM and AArch64 differ on the vceq variants
available with poly64.
For ARM, arm_neon.h contains: uint64x1_t vceq_p64 (poly64x1_t __a,
poly64x1_t __b) For AArch64, I can't see vceq_p64 in arm_neon.h? ...
Actually I've just noticed the other you submitted while I was writing
this, where you add vceq_p64 for aarch64, but it still returns
uint64_t.
Why do you change the vceq_64 test to return poly64_t instead of
uint64_t?
This patch is slightly outdated. The correct type is `uint64_t` but when it was
noticed
This patch was already sent. New one coming soon.
Why do you add #ifdef __aarch64 before vldX_p64 tests and until vsli_p64?
This is wrong, remove them. It was supposed to be around the vldX_lane_p64
tests.
The comment /* vget_lane_p64 tests. */ is wrong before VLDX_LANE
tests
You need to protect the new vmov, vget_high and vget_lane tests with
#ifdef __aarch64__.
vget_lane is already in an #ifdef, vmov you're right, but I also notice that the
test calls VDUP instead of VMOV, which explains why I didn't get a test failure.
Thanks for the feedback,
I'll get these updated.
Actually, vget_high_p64 exists on arm, so no need for the #fidef for it.
Christophe
Kind regards,
Tamar
________________________________________
From: Tamar Christina
Sent: Tuesday, November 8, 2016 11:58:46 AM
To: Christophe Lyon
Cc: GCC Patches; christophe.l...@st.com; Marcus Shawcroft; Richard
Earnshaw; James Greenhalgh; Kyrylo Tkachov; nd
Subject: RE: [AArch64][ARM][GCC][PATCHv2 3/3] Add tests for missing
Poly64_t intrinsics to GCC
Hi Christophe,
Thanks for the review!
A while ago I added p64_p128.c, to contain all the poly64/128 tests
except for vreinterpret.
Why do you need to create p64.c ?
I originally created it because I had a much smaller set of
intrinsics that I wanted to add initially, this grew and It hadn't occurred to
me that I can use the existing file now.
Another reason was the effective-target arm_crypto_ok as you
mentioned below.
Similarly, adding tests for vcreate_p64 etc... in p64.c or
p64_p128.c might be easier to maintain than adding them to vcreate.c
etc with several #ifdef conditions.
Fair enough, I'll move them to p64_p128.c.
For vdup-vmod.c, why do you add the "&& defined(__aarch64__)"
condition? These intrinsics are defined in arm/arm_neon.h, right?
They are tested in p64_p128.c
I should have looked for them, they weren't being tested before so I
had Mistakenly assumed that they weren't available. Now I realize I
just need To add the proper test option to the file to enable crypto. I'll
update this as well.
Looking at your patch, it seems some tests are currently missing for arm:
vget_high_p64. I'm not sure why I missed it when I removed neont-
testgen...
I'll adjust the test conditions so they run for ARM as well.
Regarding vreinterpret_p128.c, doesn't the existing effective-target
arm_crypto_ok prevent the tests from running on aarch64?
Yes they do, I was comparing the output against a clean version and
hasn't noticed That they weren't running. Thanks!
Thanks,
Christophe
