diff mbox

Use fabs(f/l) rather than __fabs

Message ID DB6PR0801MB2053B16BF79A3E6ECD6D7359837E0@DB6PR0801MB2053.eurprd08.prod.outlook.com
State New, archived
Headers show

Commit Message

Wilco Dijkstra Sept. 29, 2017, 1:02 p.m. UTC
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

Joseph Myers Sept. 29, 2017, 4:30 p.m. UTC | #1
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.)
Wilco Dijkstra Sept. 29, 2017, 6:34 p.m. UTC | #2
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
Joseph Myers Sept. 29, 2017, 7:56 p.m. UTC | #3
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.
Gabriel F. T. Gomes Sept. 29, 2017, 7:59 p.m. UTC | #4
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.
Szabolcs Nagy Oct. 3, 2017, 2:32 p.m. UTC | #5
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.
Szabolcs Nagy Oct. 3, 2017, 3:49 p.m. UTC | #6
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.
diff mbox

Patch

diff --git a/sysdeps/ieee754/dbl-64/e_lgamma_r.c b/sysdeps/ieee754/dbl-64/e_lgamma_r.c
index b5860d8a2419c143022eb9d6e1e0d747cbdb3e0e..93eda99662da25f38aca82a15b11d244a67335ef 100644
--- a/sysdeps/ieee754/dbl-64/e_lgamma_r.c
+++ b/sysdeps/ieee754/dbl-64/e_lgamma_r.c
@@ -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);
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;
diff --git a/sysdeps/ieee754/flt-32/e_lgammaf_r.c b/sysdeps/ieee754/flt-32/e_lgammaf_r.c
index 1b30dcd84d6849b0be5833a52e7ff1001f639ac7..b0baad6e14a930f9bb062d7446b80003845d497e 100644
--- a/sysdeps/ieee754/flt-32/e_lgammaf_r.c
+++ b/sysdeps/ieee754/flt-32/e_lgammaf_r.c
@@ -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);
diff --git a/sysdeps/ieee754/flt-32/e_log10f.c b/sysdeps/ieee754/flt-32/e_log10f.c
index 86dd9b3d96b145e9de12b8c9376991bedc8a6c22..7f1ffdad771a642d9df9d245ac5062ea11aaeefb 100644
--- a/sysdeps/ieee754/flt-32/e_log10f.c
+++ b/sysdeps/ieee754/flt-32/e_log10f.c
@@ -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 */
diff --git a/sysdeps/ieee754/flt-32/e_log2f.c b/sysdeps/ieee754/flt-32/e_log2f.c
index 782d901094a95afd6d7558b75f5066a2194904e3..04ab1bbb0267a8df6052c96d353c9fa17a694448 100644
--- a/sysdeps/ieee754/flt-32/e_log2f.c
+++ b/sysdeps/ieee754/flt-32/e_log2f.c
@@ -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 */
diff --git a/sysdeps/ieee754/ldbl-128/e_lgammal_r.c b/sysdeps/ieee754/ldbl-128/e_lgammal_r.c
index a80c9eaf33eba0c1f3ba1fd2809fbca1070a5fba..5c50e4616a94e1aeedb653043d799e9133e8a90f 100644
--- a/sysdeps/ieee754/ldbl-128/e_lgammal_r.c
+++ b/sysdeps/ieee754/ldbl-128/e_lgammal_r.c
@@ -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;
diff --git a/sysdeps/ieee754/ldbl-128/e_log10l.c b/sysdeps/ieee754/ldbl-128/e_log10l.c
index c992f6e5ee3556c0febf909852495069831445dc..e8f33984f2ceec4dfcbe51696b25595e31385c00 100644
--- a/sysdeps/ieee754/ldbl-128/e_log10l.c
+++ b/sysdeps/ieee754/ldbl-128/e_log10l.c
@@ -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)
diff --git a/sysdeps/ieee754/ldbl-128/e_log2l.c b/sysdeps/ieee754/ldbl-128/e_log2l.c
index cf4a380f16e5751a99adb6f9662295262ee5eb9b..06bf04f5e3f7d30a1ae7ebfb3b88f01398f11f65 100644
--- a/sysdeps/ieee754/ldbl-128/e_log2l.c
+++ b/sysdeps/ieee754/ldbl-128/e_log2l.c
@@ -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)
diff --git a/sysdeps/ieee754/ldbl-128ibm/e_lgammal_r.c b/sysdeps/ieee754/ldbl-128ibm/e_lgammal_r.c
index f881b8c0a4a1dd7c065f5a3a7a4750c69140d9ea..5b628bedc18075e241695c541af74722917fa9dc 100644
--- a/sysdeps/ieee754/ldbl-128ibm/e_lgammal_r.c
+++ b/sysdeps/ieee754/ldbl-128ibm/e_lgammal_r.c
@@ -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;
diff --git a/sysdeps/ieee754/ldbl-128ibm/e_log10l.c b/sysdeps/ieee754/ldbl-128ibm/e_log10l.c
index 1fbfa48e13edea258a07eb45e4aaa2ea491f307d..62e3214ca436f768e07035512dde5019ce184515 100644
--- a/sysdeps/ieee754/ldbl-128ibm/e_log10l.c
+++ b/sysdeps/ieee754/ldbl-128ibm/e_log10l.c
@@ -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)
diff --git a/sysdeps/ieee754/ldbl-128ibm/e_log2l.c b/sysdeps/ieee754/ldbl-128ibm/e_log2l.c
index c820dacf08478120dbcf3bf5d20df270909c2cc4..1f8b6e9d7f1c8d32ff954e3d63d2710bab28112d 100644
--- a/sysdeps/ieee754/ldbl-128ibm/e_log2l.c
+++ b/sysdeps/ieee754/ldbl-128ibm/e_log2l.c
@@ -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)