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

[Christophe Lyon <christophe.lyon@linaro.org>]

On 25 November 2016 at 15:53, Christophe Lyon
<christophe.lyon@linaro.org> wrote:
> Hi Tamar,
>
> On 24 November 2016 at 12:45, Tamar Christina <Tamar.Christina@arm.com> wrote:
>> Hi Christoph,
>>
>> I have combined most of the tests in p64_p128 except for the
>> vreinterpret_p128 and vreinterpret_p64 ones because I felt the number
>> of code that would be have to be added to p64_p128 vs having them in those
>> files isn't worth it. Since a lot of the test setup would have to be copied.
>>
>
> 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?
>
> * 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?
>
> Why do you add #ifdef __aarch64 before vldX_p64 tests and until vsli_p64?
>
> 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__.
>

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