This is the mail archive of the
mailing list for the glibc project.
Re: [PATCH] PPC64: First in the series of patches implementing POWER8 vector math.
- From: Joseph Myers <joseph at codesourcery dot com>
- To: GT <tnggil at protonmail dot com>
- Cc: "libc-alpha at sourceware dot org" <libc-alpha at sourceware dot org>
- Date: Tue, 19 Feb 2019 01:42:33 +0000
- Subject: Re: [PATCH] PPC64: First in the series of patches implementing POWER8 vector math.
- References: <nN0kG0GXbx7wYro2oE34bwvcauTJw_WuDyP-obLhVMc-zwTK071-mQXBD8hiITyOHI6WoxiwvNezhvebbsjFnNlAk8mCi5yX6NYpge4SAasfirstname.lastname@example.org>
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
Please also reference somewhere other than Google Groups for the ABI (both
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
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