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