Re: [PATCH] Aarch64: Add simd exp/expf functions

[Joseph Myers <joseph at codesourcery dot com>]

The following comments are mostly on issues also raised for other 
architectures, so reading the discussions of both the powerpc patches and 
the x86_64 patches is encouraged.

1. The commit message needs to reference the specification of the ABI 
being immplemented, and give confirmation of this having been agreed among 
all relevant parties, and give details of the GCC version implementing the 
ABI.  (The ABI document should be clear on exactly what function variants 
the pragma / attributes mean should be available.  If you wish to add 
other variants in future, e.g. SVE variants, those will need to use a 
*different* pragma / attribute, to avoid new compilers misinterpreting the 
headers from old glibc as meaning the CVE variants are available.)

2. There needs to be a NEWS entry describing the new user-visible feature 
and also giving details of the GCC version with support.

3. There should not be any _finite aliases exported from the shared 
library; rather, use static-only wrappers as on x86_64.  Or fix the 
underlying GCC issue to allow the asm name used as a basis for vector 
function names to be different from that used as a scalar function name; 
see <https://gcc.gnu.org/ml/gcc/2015-06/msg00173.html>.

4. There are formatting issues in this code, including missing spaces 
before '(' and incorrect indentation.

5. Give details (including test programs) of how you tested that the 
functions do work, with an installed glibc and new-enough GCC, for 
vectorized calls resulting from source code calling the scalar functions, 
which the glibc testsuite doesn't cover.  This is important end-to-end 
validation that the ABI is as intended; the lack of it for x86_64 resulted 
in sincos ABI issues only being found later.

6. What does if('aarch64') in the Fortran header mean?  What do you need 
it at all?  The installed header should work for all AArch64 ABIs (so 
currently BE and LE); it's not expected to work for other architectures.

7. "#ifdef BIG_ENDIAN" is not a valid conditional.  The endian.h header 
defines both BIG_ENDIAN and LITTLE_ENDIAN, and then defines BYTE_ORDER to 
one of those.  Does libmvec_util.h get an implicit include of endian.h 
somewhere (so you always get the BE path, which somehow works on LE, 
indicating test coverage issues that should be resolved, preferably 
through automated tests but failing that please describe in the commit 
message how you tested that the endian conditionals were correct), or have 
you only tested for little-endian which worked because of the macro being 
accidentally undefined?

8. Please confirm in the commit message how testing was run for both BE 
and LE, given the presence of such conditionals.

-- 
Joseph S. Myers
joseph@codesourcery.com