This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [AArch64][ARM][GCC][PATCHv2 3/3] Add tests for missing Poly64_t intrinsics to GCC
On 29 November 2016 at 11:12, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> Hi Tamar,
>
>
> On 29 November 2016 at 10:50, Tamar Christina <Tamar.Christina@arm.com> 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.
>>
>
> LGTM.
>
> One more question: maybe we want to add explicit tests for vdup*_v_p64
> even though they are aliases for vmov?
>
Sorry, I meant vdup_n_p64, but the tests are already in place.
So, OK for me, but I can't approve.
Thanks,
Christophe
> Christophe
>
>> Kind Regards,
>> Tamar
>>
>> ________________________________________
>> From: Tamar Christina
>> Sent: Friday, November 25, 2016 4:01:30 PM
>> To: Christophe Lyon
>> Cc: GCC Patches; christophe.lyon@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.lyon@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