Hi Tamar, On 24 November 2016 at 12:45, Tamar Christina <tamar.christ...@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__. 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
