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

[GT <tnggil at protonmail dot com>]

‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
On Thursday, February 14, 2019 9:17 PM, Joseph Myers <joseph@codesourcery.com> wrote:

> If you have a copyright assignment / employer disclaimer on file at the
> FSF, could you give details of the names involved?
>

I emailed an electronically signed copyright assignment to assign@gnu.org a little while ago. The name is Bert Tenjy and there are no employers/schools/other who could claim any contributions by me.

> >         * sysdeps/powerpc/powerpc64/multiarch/test-double-vlen2.c: New file.
> >
>
> Why? x86_64 doesn't have this.
>

I have been shadowing the sequence of function implementation and testing done by x86_64. Looking further ahead, there was a significant reorganization of the testing procedure which removed multiple files, including the one in question here. I will rewrite PPC64 testing to fit into the current testing infrastructure.

> > +#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)?
>

To address the support issues raised, a solution would be to have 'configure' verify that the compiler generates a valid executable. Then _ARCH_PPC64 would be replaced here by a macro determined at configuration time.

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

Will ensure no such blank lines in any file.

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

This has to do with the code not using the latest testing infrastructure. Fixing that should either preserve this file with overrides or eliminate its inclusion.

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

Will look through the standard and adhere.

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

Will fix.

===========
Thank.
Bert Tenjy.