This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] PowerPC - ilogb[f|l] optimization for POWER7


On Tue, May 8, 2012 at 7:33 AM, Adhemerval Zanella
<azanella@linux.vnet.ibm.com> wrote:
> This patch provides optimized ilogb (60% on PPC32 and 20% PPC64),
> ilogbf (60% on PPC32 and 50% on PPC64), and ilogbl (3% on PPC32
> and 8% on PPC64). The optimization is done by avoiding float-point
> to integer transformation and by using VSX float-point bitwise
> instructions.

Please indicate why you chose to implement the changes the way you did
by removing e_ilogb.

> ---
>
> 2012-05-08 ÂAdhemerval Zanella Â<azanella@linux.vnet.ibm.com>
>
> Â Â Â Â* sysdeps/powerpc/powerpc32/power7/fpu/w_ilogb.c: New file: optimized
> Â Â Â Âilogb for POWER7.
> Â Â Â Â* sysdeps/powerpc/powerpc32/power7/fpu/w_ilogbf.c: New file: optimized
> Â Â Â Âilogbf for POWER7.
> Â Â Â Â* sysdeps/powerpc/powerpc32/power7/fpu/w_ilogbl.c: New file: optimized
> Â Â Â Âilogbl for POWER7.
> Â Â Â Â* sysdeps/powerpc/powerpc64/power7/fpu/w_ilogb.c: New file: wrapper
> Â Â Â Âfor the optimized logb for PPC64.
> Â Â Â Â* sysdeps/powerpc/powerpc64/power7/fpu/w_ilogbf.c: New file: wrapper
> Â Â Â Âfor the optimized logbf for PPC64.
> Â Â Â Â* sysdeps/powerpc/powerpc64/power7/fpu/w_ilogbl.c: New file: wrapper
> Â Â Â Âfor the optimized logbl for PPC64.

Same comment as I had on the logb fixes.  Please indicate that this
uses the powerpc32 version.

> Â Â Â Â* sysdeps/powerpc/powerpc32/power7/fpu/e_ilogb.c: New file: black file
> Â Â Â Âto avoid compilation of default implementation.

I assume you mean "blank" file.. but I prefer something like "dummy"
file because it's not exactly blank/empty.

> Â Â Â Â* sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbf.c: Likewise.
> Â Â Â Â* sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbl.c: Likewise.
> Â Â Â Â* sysdeps/powerpc/powerpc64/power7/fpu/e_ilogb.c: Likewise.
> Â Â Â Â* sysdeps/powerpc/powerpc64/power7/fpu/e_ilogbf.c: Likewise.
> Â Â Â Â* sysdeps/powerpc/powerpc64/power7/fpu/e_ilogbl.c: Likewise.
> Â Â Â Â* sysdeps/powerpc/fpu/math_private.h: Add Optimized double to word
> Â Â Â Âcast for POWER7.
> Â Â Â Â* math/libm-test.inc (ilogb_test): New ilogb tests.
>
> diff --git a/math/libm-test.inc b/math/libm-test.inc
> index 542131d..677e5e9 100644
> --- a/math/libm-test.inc
> +++ b/math/libm-test.inc
> @@ -4082,6 +4082,10 @@ ilogb_test (void)
> Â TEST_f_i (ilogb, M_El, 1);
> Â TEST_f_i (ilogb, 1024, 10);
> Â TEST_f_i (ilogb, -2000, 10);
> + ÂTEST_f_i (ilogb, 1.701412e+38, 127);
> +#ifndef TEST_FLOAT
> + ÂTEST_f_i (ilogb, 8.988466e+307, 1023);
> +#endif
>
> Â /* ilogb (0.0) == FP_ILOGB0 plus invalid exception Â*/
> Â errno = 0;
> diff --git a/sysdeps/powerpc/fpu/math_private.h b/sysdeps/powerpc/fpu/math_private.h
> index a916be3..aa3053a 100644
> --- a/sysdeps/powerpc/fpu/math_private.h
> +++ b/sysdeps/powerpc/fpu/math_private.h
> @@ -25,6 +25,29 @@
> Â#include <dl-procinfo.h>
> Â#include_next <math_private.h>
>
> +#if defined(_ARCH_PWR7)
> +
> +/* Optimized double to word cast for POWER7: the 'ori 2,2,0'
> + Â instructions between the store double / load integer is
> + Â to force a new dispatch group. Â*/
> +#undef DOUBLE_TO_WORDS
> +#define DOUBLE_TO_WORDS(d, i) Â Â Â Â Â Â Â Â Â Â Â Â Â\
> + Âdo { Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â Âdouble d__ = d; Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> + Â Âint32_t i__; Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â Âieee_double_shape_type iw_u; Â Â Â Â Â Â Â Â Â Â Â \
> + Â Â__asm ( Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> + Â Â Â"fctiwz Â%1,%1\n" Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â\
> +   Â"stfd  Â%1,%2\n"                    Â\
> +   Â"ori   2,2,0\n"                    Â\
> +   Â"lwz   %0,%3\n"                    Â\
> + Â Â Â: "=r" (i__) Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â Â Â: "f" (d__), "m" (iw_u.value), "m" (iw_u.word)); \
> + Â Âi = i__; Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â \
> + Â} while (0)
> +
> +#endif /* __ARCH_PWR7 Â*/
> +
> Â# if __WORDSIZE == 64 || defined _ARCH_PWR4
> Â# Âdefine __CPU_HAS_FSQRT 1
> Â# else
> diff --git a/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogb.c b/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogb.c
> new file mode 100644
> index 0000000..34f1a9b
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogb.c
> @@ -0,0 +1 @@
> +/* ilogb implementation is at w_ilogb.c Â*/

I think perhaps "in" is more correct in this case since the
implementation is contained within that particular file.

> diff --git a/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbf.c b/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbf.c
> new file mode 100644
> index 0000000..1bbe157
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbf.c
> @@ -0,0 +1 @@
> +/* ilogbf implementation is at w_ilogbf.c Â*/
> diff --git a/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbl.c b/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbl.c
> new file mode 100644
> index 0000000..7684390
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc32/power7/fpu/e_ilogbl.c
> @@ -0,0 +1 @@
> +/* ilogbl implementation is at w_ilogbl.c Â*/
> diff --git a/sysdeps/powerpc/powerpc32/power7/fpu/w_ilogb.c b/sysdeps/powerpc/powerpc32/power7/fpu/w_ilogb.c
> new file mode 100644
> index 0000000..525f76b
> --- /dev/null
> +++ b/sysdeps/powerpc/powerpc32/power7/fpu/w_ilogb.c
> @@ -0,0 +1,83 @@
> +/* ilogb(). PowerPC64/POWER7 version.
> + Â Copyright (C) 2012 Free Software Foundation, Inc.
> + Â Contributed by Adhemerval Zanella Netto <azanella@br.ibm.com>.

Remove the Contributed by line, here and elsewhere.

> + Â 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 <fenv.h>
> +#include <errno.h>
> +#include <math.h>
> +#include <math_private.h>
> +
> +

Nit, remove extra empty line ^^^ in this file an others.

I have no further comments.

I technically it's OK.

Ryan


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]