Re: [PATCH] PPC64: First in the series of patches implementing POWER8 vector math.

[Joseph Myers <joseph at codesourcery dot com>]

Please include the patch description in the body of your message, not just 
in an attachment - "Empty Message" isn't helpful.  When sending multiple 
patch versions, it's very helpful to describe what has changed since the 
previous version (after a "---" line or some other such indication, 
accepted by "git am", that the following text is not intended as part of 
the commit message).

I think you need to have explicit buy-in from powerpc toolchain 
maintainers that the ABI you have chosen is the desired one for this 
purpose, agreed for use by any toolchain that wishes to be able to use 
this functionality.  Please also confirm the GCC version that implements 
the ABI in question (when given the pragmas / attributes in the header you 
add).  To be clear, this information should be part of the commit message 
(in every version of the patch submission), not just in a one-off reply to 
this message.

Please also reference somewhere other than Google Groups for the ABI (both 
because Google Groups requires non-free JavaScript, contrary to GNU 
principles, and because Google Groups has a history of breaking URLs that 
used to work for linking to messages, so somewhere with a more reliably 
stable URL is important).  The glibc wiki has a copy of the x86_64 ABI at 
<https://sourceware.org/glibc/wiki/libmvec?action=AttachFile&do=view&target=VectorABI.txt>.

Please describe in patch submissions how the patch was tested.  In this 
case, running at least the libm tests for both powerpc64 big-endian and 
powerpc64 little-endian, and verifying there are no failures, would seem 
appropriate.  But more tests are needed that the installed header really 
does work as expected (see below).

You seem to build both VSX and AltiVec versions of the functions - is that 
correct?  But I don't see any Makefile code that actually causes the 
versions intended to be VSX versions to be built with -mvsx.

What do you intend to happen with the tests (test the VSX version, test 
the AltiVec version, return without running tests) in each of the 
following cases: running on VSX hardware, running on non-VSX hardware with 
AltiVec, running on hardware without either?  How do you ensure that?  
(x86_64 has math-tests-arch.h to define CHECK_ARCH_EXT to avoid running 
tests on unsupported hardware.)

What happens if you build for hardware without AltiVec?  It's of course 
fine for these functions to have an ABI that depends on AltiVec (so they 
never get called on such hardware) - do they need an explicit -maltivec 
option to ensure the vector types / ABI are available, or do they build OK 
for the correct ABI even without those options?

You're now testing a macro __POWERPC64__ in the header, but GCC doesn't 
predefine that macro, only __powerpc64__.  You need to test properly that 
the header you have really does work to cause appropriately built code, 
using the installed headers, to call these functions.  (glibc's own 
testsuite doesn't verify that, only that the internal functions work as 
expected by their mangled names.  So you need appropriate manual tests of 
using installed glibc with a suitable compiler to make sure calls get 
vectorized as expected and the vectorized calls work - and you should give 
details of that testing in the proposed commit message, to demonstrate 
that the patch has been sufficiently tested.)

A new feature like this needs a NEWS entry.

There are still coding style issues in the patch.  Comments inside a 
function are expected to be appropriately indented to match the code in 
the function, not have the "/*" at the left margin.  "if" blocks only 
containing a single statement should generally not use braces around it.  
C++-style "//" comments are not used.  Comments should end with ".  " 
(full stop, two spaces, end of comment), and start with a capital letter.

-- 
Joseph S. Myers
joseph@codesourcery.com