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

[GT <tnggil at protonmail dot com>]

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

This is the testing being done. I will add the description.

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

There do need to be separate VSX and Altivec versions. Power8 hardware
automatically enables VSX functionality. But I will add the -mvsx flag to
the Makefile for VSX version builds occurring on non-Power8 systems.
And also add -maltivec for the Altivec version.

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

There is a runtime test in vec_d_cos2_core.c which currently selects
between the VSX and Altivec versions. It reads hwcap and makes
the choice depending on the result returned for supported ISA. On
systems with no Altivec and no VSX, my intention is to fall back to
calling the scalar cosine twice and run the test anyway.

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

GCC fails if an attempt is made to compile code that uses Altivec
functionality without passing the -maltivec flag. Additionally it also
fails on attempts to build for pre-POWER7 hardware even when
-maltivec flag is given to the compiler. Is there need to add a
check in configure for attempts to build libmvec on non-Altivec, non-VSX
systems; or let the failure happen when make calls GCC?

> You're now testing a macro __POWERPC64__ in the header, but GCC doesn't
> predefine that macro, only __powerpc64__.

You are right. I will make the change.

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

As I understand the issue here, the only vector function that can be called
after glibc is built with this patch is the double-precision vector cosine.
Automatic vectorization of functions is a GCC feature that is as yet
unimplemented.

I will follow the instructions on how to test a new installed glibc given here:
https://sourceware.org/glibc/wiki/Testing/Builds

Even then only the vector cosine can be tested. Unless I've missed a
signficant point.

> A new feature like this needs a NEWS entry.
>

Will add an entry similar to that when x86_64 implementation commenced.

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

Will be fixed.