[AArch64][ARM][GCC][PATCHv2 3/3] Add tests for missing Poly64_t intrinsics to GCC

[Kyrill Tkachov]

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.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