This is the mail archive of the
libc-alpha@sourceware.org
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: Thu, 14 Feb 2019 21:17:20 +0000
- Subject: Re: [PATCH] PPC64: First in the series of patches implementing POWER8 vector math.
- References: <7z03WSXwPIOYABvW1ZYzHobMOv90wBWKo-e0YptRFovFUpRnrSEGN-zv08kOe_fPJMFnHx1khbcfcfooubTStEAtMX96Q2U4cRVZOm_TZl8=@protonmail.com>
If you have a copyright assignment / employer disclaimer on file at the
FSF, could you give details of the names involved?
> * sysdeps/powerpc/powerpc64/multiarch/test-double-vlen2.c: New file.
Why? x86_64 doesn't have this.
> +#if defined _ARCH_PPC64 && defined __FAST_MATH__
Is _ARCH_PPC64 correct here - what's the status of support (in the GNU
toolchain, Linux kernel, etc.) for -mpowerpc64 with the 32-bit ABI (which
also defines _ARCH_PPC64)?
> diff --git a/sysdeps/powerpc/powerpc64/fpu/Versions b/sysdeps/powerpc/powerpc64/fpu/Versions
> new file mode 100644
> index 0000000000..9da7f92ffe
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/fpu/Versions
> @@ -0,0 +1,6 @@
> +libmvec {
> + GLIBC_2.30 {
> + _ZGVbN2v_cos;
> + }
> +}
> +
You can't push commits that add blank lines at end of files; the
repository blocks such pushes.
> diff --git a/sysdeps/powerpc/powerpc64/fpu/multiarch/math-tests-arch.h b/sysdeps/powerpc/powerpc64/fpu/multiarch/math-tests-arch.h
> new file mode 100644
> index 0000000000..e79b98480b
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc64/fpu/multiarch/math-tests-arch.h
> @@ -0,0 +1,19 @@
> +/*
> + Copyright (C) 2019 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <http://www.gnu.org/licenses/>. */
> +
> +#include <sysdeps/generic/math-tests-arch.h>
This file is useless (the only point of a sysdeps file that just includes
the generic version would be to override a file in another sysdeps
directory that would otherwise be used).
> +vector double _ZGVbN2v_cos_vsx (vector double x)
> +{
> +
> +/*
> + ARGUMENT RANGE REDUCTION:
> + Add Pi/2 to argument: X' = X+Pi/2
> + */
> + vector double x_prime = (vector double) *dHalfPI + x;
Code formatting needs fixing to follow the GNU Coding Standards, here and
elsewhere in the patch.
> +#include <altivec.h>
> +
> +vector unsigned long long dHalfPI_t =
> +{0x3ff921fb54442d18,0x3ff921fb54442d18};
There are namespace problems here. All external constants should have
names in the implementation namespace, meaning two leading underscores.
They should also be const; don't add new writable global variables. In
addition, use vector double and hex floats, rather than writing out the
integer representation. In addition, each constant should have a comment
explaining its semantics, sufficiently precisely that someone could
recompute the value and verify its correctness.
--
Joseph S. Myers
joseph@codesourcery.com