On 29 November 2016 at 11:12, Christophe Lyon <christophe.l...@linaro.org> wrote: > Hi Tamar, > > > On 29 November 2016 at 10:50, Tamar Christina <tamar.christ...@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.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