Use fabs(f/l) rather than __fabs
Commit Message
A few math functions still use __fabs(f/l) rather than fabs, which
means they won't be inlined. Rename them so they are inlined.
Passes GLIBC tests on AArch64 - no calls to __fabs(f/l).
Note given ldbl-128 and ldbl-128ibm already contain uses of fabsl,
there is no risk of introducing localplt or namespace issues.
OK for commit?
ChangeLog:
2017-09-29 Wilco Dijkstra <wdijkstr@arm.com>
* sysdeps/ieee754/dbl-64/e_lgamma_r.c
(__ieee754_lgamma_r): Use fabs rather than __fabs.
* sysdeps/ieee754/dbl-64/e_log10.c (__ieee754_log10): Likewise.
* sysdeps/ieee754/dbl-64/e_log2.c (__ieee754_log2): Likewise.
* sysdeps/ieee754/flt-32/e_lgammaf_r.c
(__ieee754_lgammaf_r): Use fabsf rather than __fabsf.
* sysdeps/ieee754/flt-32/e_log10f.c (__ieee754_log10f): Likewise.
* sysdeps/ieee754/flt-32/e_log2f.c (__ieee754_log2f): Likewise.
* sysdeps/ieee754/ldbl-128/e_lgammal_r.c
(__ieee754_lgammal_r): Use fabsl rather than __fabsl.
* sysdeps/ieee754/ldbl-128/e_log10l.c (__ieee754_log10l): Likewise.
* sysdeps/ieee754/ldbl-128/e_log2l.c (__ieee754_log2l): Likewise.
* sysdeps/ieee754/ldbl-128ibm/e_lgammal_r.c
(__ieee754_lgammal_r): Use fabsl rather than __fabsl.
* sysdeps/ieee754/ldbl-128ibm/e_log10l.c (__ieee754_log10l): Likewise.
* sysdeps/ieee754/ldbl-128ibm/e_log2l.c (__ieee754_log2l): Likewise.
--
Comments
On Fri, 29 Sep 2017, Wilco Dijkstra wrote:
> A few math functions still use __fabs(f/l) rather than fabs, which
> means they won't be inlined. Rename them so they are inlined.
> Passes GLIBC tests on AArch64 - no calls to __fabs(f/l).
>
> Note given ldbl-128 and ldbl-128ibm already contain uses of fabsl,
> there is no risk of introducing localplt or namespace issues.
I think you need to add -fno-builtin-fabsl for e_lgammal_r.c, e_log10l.c
and e_log2l.c in sysdeps/powerpc/nofpu/Makefile, for the same reason as
the existing -fno-builtin-fabsl uses there - the built-in fabsl does not
work properly for signed zero for powerpc soft-float / e500v1 (and the
uses of __fabsl being changed are all uses that need to work correctly for
signed zero).
OK with that change.
To be clear, the reason the change is safe regarding namespace issues is
that the particular functions in question don't get called from C90
functions in the case where fabsl might not be inlined (which is only the
powerpc soft-float / e500v1 case where -fno-builtin-fabsl is needed); the
existence of fabsl calls from other functions is not sufficient. There is
code to call ldbl-128ibm expl from pow, but that's not one of the affected
functions and that code doesn't apply for soft-float / e500v1 anyway.
(fabsl is in fact a reserved name in C90, but the linknamespace tests
don't know that.)
Joseph Myers wrote:
> I think you need to add -fno-builtin-fabsl for e_lgammal_r.c, e_log10l.c
> and e_log2l.c in sysdeps/powerpc/nofpu/Makefile, for the same reason as
> the existing -fno-builtin-fabsl uses there - the built-in fabsl does not
> work properly for signed zero for powerpc soft-float / e500v1 (and the
> uses of __fabsl being changed are all uses that need to work correctly for
> signed zero).
Done, I verified that glibcs-powerpc-linux-gnu-soft now builds those files with
-fno-builtin-fabsl and that it passes.
> To be clear, the reason the change is safe regarding namespace issues is
> that the particular functions in question don't get called from C90
> functions in the case where fabsl might not be inlined (which is only the
> powerpc soft-float / e500v1 case where -fno-builtin-fabsl is needed); the
> existence of fabsl calls from other functions is not sufficient. There is
> code to call ldbl-128ibm expl from pow, but that's not one of the affected
> functions and that code doesn't apply for soft-float / e500v1 anyway.
> (fabsl is in fact a reserved name in C90, but the linknamespace tests
> don't know that.)
Right so there are no namespace issues. But what about localplt issues
due to the disabling of fabsl inlining? I can't find any renaming, so why are
there no local plt failures on ppc soft-float?
Wilco
On Fri, 29 Sep 2017, Wilco Dijkstra wrote:
> Right so there are no namespace issues. But what about localplt issues
> due to the disabling of fabsl inlining? I can't find any renaming, so why are
> there no local plt failures on ppc soft-float?
Because both fabsl and copysignl are listed in
sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/localplt.data. If the GCC
bug is fixed and the -fno-builtin-fabsl use made conditional, then fabsl
would also need to be marked optional in localplt.data as copysignl is.
On 29 Sep 2017, Wilco Dijkstra wrote:
>Right so there are no namespace issues. But what about localplt issues
>due to the disabling of fabsl inlining? I can't find any renaming, so why are
>there no local plt failures on ppc soft-float?
Because sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/localplt.data,
the localplt.data file for ppc soft-float, lists fabsl as a required entry.
On 29/09/17 14:02, Wilco Dijkstra wrote:
> diff --git a/sysdeps/ieee754/dbl-64/e_log10.c b/sysdeps/ieee754/dbl-64/e_log10.c
> index dfb7d056caee5f592048559b448c899aa11e9de2..677cbc4df8739521c50a72d5ab657f438c74c129 100644
> --- a/sysdeps/ieee754/dbl-64/e_log10.c
> +++ b/sysdeps/ieee754/dbl-64/e_log10.c
> @@ -65,7 +65,7 @@ __ieee754_log10 (double x)
> if (hx < 0x00100000)
> { /* x < 2**-1022 */
> if (__glibc_unlikely (((hx & 0x7fffffff) | lx) == 0))
> - return -two54 / __fabs (x); /* log(+-0)=-inf */
> + return -two54 / fabs (x); /* log(+-0)=-inf */
> if (__glibc_unlikely (hx < 0))
> return (x - x) / (x - x); /* log(-#) = NaN */
> k -= 54;
> diff --git a/sysdeps/ieee754/dbl-64/e_log2.c b/sysdeps/ieee754/dbl-64/e_log2.c
> index 2f3da129f83dd0dc2fc7da78cfce3804febdbac5..e4a6aff9a3b5aa68c03603bf4cb8bb78507ea9c6 100644
> --- a/sysdeps/ieee754/dbl-64/e_log2.c
> +++ b/sysdeps/ieee754/dbl-64/e_log2.c
> @@ -83,7 +83,7 @@ __ieee754_log2 (double x)
> if (hx < 0x00100000)
> { /* x < 2**-1022 */
> if (__glibc_unlikely (((hx & 0x7fffffff) | lx) == 0))
> - return -two54 / __fabs (x); /* log(+-0)=-inf */
> + return -two54 / fabs (x); /* log(+-0)=-inf */
> if (__glibc_unlikely (hx < 0))
> return (x - x) / (x - x); /* log(-#) = NaN */
> k -= 54;
note that on aarch64 wrong code is generated for these fabs
changes, most likely a gcc bug, still investigating.
it is not visible on aarch64 because the compat wrapper
handles the log(0) case, but without the wrapper (e.g. ilp32)
log(0) in downward rounding becomes +inf instead of -inf,
because fabs(x) is compiled to x-x for x==0 for some reason.
On 03/10/17 15:32, Szabolcs Nagy wrote:
> On 29/09/17 14:02, Wilco Dijkstra wrote:
>> diff --git a/sysdeps/ieee754/dbl-64/e_log10.c b/sysdeps/ieee754/dbl-64/e_log10.c
>> index dfb7d056caee5f592048559b448c899aa11e9de2..677cbc4df8739521c50a72d5ab657f438c74c129 100644
>> --- a/sysdeps/ieee754/dbl-64/e_log10.c
>> +++ b/sysdeps/ieee754/dbl-64/e_log10.c
>> @@ -65,7 +65,7 @@ __ieee754_log10 (double x)
>> if (hx < 0x00100000)
>> { /* x < 2**-1022 */
>> if (__glibc_unlikely (((hx & 0x7fffffff) | lx) == 0))
>> - return -two54 / __fabs (x); /* log(+-0)=-inf */
>> + return -two54 / fabs (x); /* log(+-0)=-inf */
>> if (__glibc_unlikely (hx < 0))
>> return (x - x) / (x - x); /* log(-#) = NaN */
>> k -= 54;
>> diff --git a/sysdeps/ieee754/dbl-64/e_log2.c b/sysdeps/ieee754/dbl-64/e_log2.c
>> index 2f3da129f83dd0dc2fc7da78cfce3804febdbac5..e4a6aff9a3b5aa68c03603bf4cb8bb78507ea9c6 100644
>> --- a/sysdeps/ieee754/dbl-64/e_log2.c
>> +++ b/sysdeps/ieee754/dbl-64/e_log2.c
>> @@ -83,7 +83,7 @@ __ieee754_log2 (double x)
>> if (hx < 0x00100000)
>> { /* x < 2**-1022 */
>> if (__glibc_unlikely (((hx & 0x7fffffff) | lx) == 0))
>> - return -two54 / __fabs (x); /* log(+-0)=-inf */
>> + return -two54 / fabs (x); /* log(+-0)=-inf */
>> if (__glibc_unlikely (hx < 0))
>> return (x - x) / (x - x); /* log(-#) = NaN */
>> k -= 54;
>
>
> note that on aarch64 wrong code is generated for these fabs
> changes, most likely a gcc bug, still investigating.
>
> it is not visible on aarch64 because the compat wrapper
> handles the log(0) case, but without the wrapper (e.g. ilp32)
> log(0) in downward rounding becomes +inf instead of -inf,
> because fabs(x) is compiled to x-x for x==0 for some reason.
>
sorry this analysis was wrong and has nothing to do with
the fabs change only the svid compat change matters.
it is a real bug in log2 and log10 of wordsize-64/
that is masked by the wrapper in the test system.
@@ -225,7 +225,7 @@ __ieee754_lgamma_r(double x, int *signgamp)
if(hx<0) {
if(__builtin_expect(ix>=0x43300000, 0))
/* |x|>=2**52, must be -integer */
- return __fabs (x)/zero;
+ return fabs (x)/zero;
if (x < -2.0 && x > -28.0)
return __lgamma_neg (x, signgamp);
t = sin_pi(x);
@@ -65,7 +65,7 @@ __ieee754_log10 (double x)
if (hx < 0x00100000)
{ /* x < 2**-1022 */
if (__glibc_unlikely (((hx & 0x7fffffff) | lx) == 0))
- return -two54 / __fabs (x); /* log(+-0)=-inf */
+ return -two54 / fabs (x); /* log(+-0)=-inf */
if (__glibc_unlikely (hx < 0))
return (x - x) / (x - x); /* log(-#) = NaN */
k -= 54;
@@ -83,7 +83,7 @@ __ieee754_log2 (double x)
if (hx < 0x00100000)
{ /* x < 2**-1022 */
if (__glibc_unlikely (((hx & 0x7fffffff) | lx) == 0))
- return -two54 / __fabs (x); /* log(+-0)=-inf */
+ return -two54 / fabs (x); /* log(+-0)=-inf */
if (__glibc_unlikely (hx < 0))
return (x - x) / (x - x); /* log(-#) = NaN */
k -= 54;
@@ -160,7 +160,7 @@ __ieee754_lgammaf_r(float x, int *signgamp)
}
if(hx<0) {
if(ix>=0x4b000000) /* |x|>=2**23, must be -integer */
- return __fabsf (x)/zero;
+ return fabsf (x)/zero;
if (ix > 0x40000000 /* X < 2.0f. */
&& ix < 0x41700000 /* X > -15.0f. */)
return __lgamma_negf (x, signgamp);
@@ -34,7 +34,7 @@ __ieee754_log10f(float x)
k=0;
if (hx < 0x00800000) { /* x < 2**-126 */
if (__builtin_expect((hx&0x7fffffff)==0, 0))
- return -two25/__fabsf (x); /* log(+-0)=-inf */
+ return -two25/fabsf (x); /* log(+-0)=-inf */
if (__builtin_expect(hx<0, 0))
return (x-x)/(x-x); /* log(-#) = NaN */
k -= 25; x *= two25; /* subnormal number, scale up x */
@@ -43,7 +43,7 @@ __ieee754_log2f(float x)
k=0;
if (ix < 0x00800000) { /* x < 2**-126 */
if (__builtin_expect((ix&0x7fffffff)==0, 0))
- return -two25/__fabsf (x); /* log(+-0)=-inf */
+ return -two25/fabsf (x); /* log(+-0)=-inf */
if (__builtin_expect(ix<0, 0))
return (x-x)/(x-x); /* log(-#) = NaN */
k -= 25; x *= two25; /* subnormal number, scale up x */
@@ -778,7 +778,7 @@ __ieee754_lgammal_r (_Float128 x, int *signgamp)
q = -x;
p = __floorl (q);
if (p == q)
- return (one / __fabsl (p - p));
+ return (one / fabsl (p - p));
_Float128 halfp = p * L(0.5);
if (halfp == __floorl (halfp))
*signgamp = -1;
@@ -187,7 +187,7 @@ __ieee754_log10l (_Float128 x)
/* Test for domain */
GET_LDOUBLE_WORDS64 (hx, lx, x);
if (((hx & 0x7fffffffffffffffLL) | lx) == 0)
- return (-1 / __fabsl (x)); /* log10l(+-0)=-inf */
+ return (-1 / fabsl (x)); /* log10l(+-0)=-inf */
if (hx < 0)
return (x - x) / (x - x);
if (hx >= 0x7fff000000000000LL)
@@ -181,7 +181,7 @@ __ieee754_log2l (_Float128 x)
/* Test for domain */
GET_LDOUBLE_WORDS64 (hx, lx, x);
if (((hx & 0x7fffffffffffffffLL) | lx) == 0)
- return (-1 / __fabsl (x)); /* log2l(+-0)=-inf */
+ return (-1 / fabsl (x)); /* log2l(+-0)=-inf */
if (hx < 0)
return (x - x) / (x - x);
if (hx >= 0x7fff000000000000LL)
@@ -728,7 +728,7 @@ __ieee754_lgammal_r (long double x, int *signgamp)
q = -x;
p = __floorl (q);
if (p == q)
- return (one / __fabsl (p - p));
+ return (one / fabsl (p - p));
long double halfp = p * 0.5L;
if (halfp == __floorl (halfp))
*signgamp = -1;
@@ -189,7 +189,7 @@ __ieee754_log10l (long double x)
xhi = ldbl_high (x);
EXTRACT_WORDS64 (hx, xhi);
if ((hx & 0x7fffffffffffffffLL) == 0)
- return (-1.0L / __fabsl (x)); /* log10l(+-0)=-inf */
+ return (-1.0L / fabsl (x)); /* log10l(+-0)=-inf */
if (hx < 0)
return (x - x) / (x - x);
if (hx >= 0x7ff0000000000000LL)
@@ -183,7 +183,7 @@ __ieee754_log2l (long double x)
xhi = ldbl_high (x);
EXTRACT_WORDS64 (hx, xhi);
if ((hx & 0x7fffffffffffffffLL) == 0)
- return (-1.0L / __fabsl (x)); /* log2l(+-0)=-inf */
+ return (-1.0L / fabsl (x)); /* log2l(+-0)=-inf */
if (hx < 0)
return (x - x) / (x - x);
if (hx >= 0x7ff0000000000000LL)