[v2] Remove isinf uses that rely on signed return value
Commit Message
I found a few more cases where isinf is used to get the signbit. Clean these up to improve the
readability and maintainability and allow inlining. Generated code is virtually identical once isinf
is inlined using the GCC builtin.
OK for commit?
Wilco
2015-08-20 Wilco Dijkstra <wdijkstr@arm.com>
* math/w_tgamma.c (__ieee754_gamma_r): Use explicit sign check.
* math/w_tgammaf.c (__ieee754_gammaf_r): Likewise.
* math/w_tgammal.c (__ieee754_gammal_r): Likewise.
* stdio-common/printf_fp.c (___printf_fp):
Use signbit to get the sign. Use isinf macro to allow inlining.
* stdio-common/printf_fphex.c (__printf_fphex): Likewise.
* stdio-common/printf_size.c (__printf_size): Likewise.c
---
math/w_tgamma.c | 2 +-
math/w_tgammaf.c | 2 +-
math/w_tgammal.c | 2 +-
stdio-common/printf_fp.c | 13 +++++--------
stdio-common/printf_fphex.c | 10 +++-------
stdio-common/printf_size.c | 19 +++++++++----------
6 files changed, 20 insertions(+), 28 deletions(-)
Comments
On Thu, 20 Aug 2015, Wilco Dijkstra wrote:
> I found a few more cases where isinf is used to get the signbit. Clean
> these up to improve the readability and maintainability and allow
> inlining. Generated code is virtually identical once isinf is inlined
> using the GCC builtin.
>
> OK for commit?
What exactly are the dependencies of this patch and their review state?
My understanding is that first we need to get in benchmarks for the
classification macros. Then, given such benchmarks checked in, the change
to use the built-in functions can be considered on the basis of evidence
from those benchmarks. Then, with the built-in functions in use, we can
consider patches like this based on such comparisons of code generated.
> Joseph Myers wrote:
> On Thu, 20 Aug 2015, Wilco Dijkstra wrote:
>
> > I found a few more cases where isinf is used to get the signbit. Clean
> > these up to improve the readability and maintainability and allow
> > inlining. Generated code is virtually identical once isinf is inlined
> > using the GCC builtin.
> >
> > OK for commit?
>
> What exactly are the dependencies of this patch and their review state?
>
> My understanding is that first we need to get in benchmarks for the
> classification macros. Then, given such benchmarks checked in, the change
> to use the built-in functions can be considered on the basis of evidence
> from those benchmarks. Then, with the built-in functions in use, we can
> consider patches like this based on such comparisons of code generated.
That's correct. The inlining patch was already approved, so everything
is waiting for an OK for the latest version of the math-inlines benchmark
(https://sourceware.org/ml/libc-alpha/2015-08/msg00824.html).
However there is no strict dependency here - this patch will work correctly
without inlining of isinf and cause no regressions for typical inputs.
Wilco
@@ -26,7 +26,7 @@ __tgamma(double x)
double y = __ieee754_gamma_r(x,&local_signgam);
if(__glibc_unlikely (!isfinite (y) || y == 0)
- && (isfinite (x) || isinf (x) < 0)
+ && (isfinite (x) || (isinf (x) && x < 0.0))
&& _LIB_VERSION != _IEEE_) {
if (x == 0.0)
return __kernel_standard(x,x,50); /* tgamma pole */
@@ -24,7 +24,7 @@ __tgammaf(float x)
float y = __ieee754_gammaf_r(x,&local_signgam);
if(__glibc_unlikely (!isfinite (y) || y == 0)
- && (isfinite (x) || isinf (x) < 0)
+ && (isfinite (x) || (isinf (x) && x < 0.0))
&& _LIB_VERSION != _IEEE_) {
if (x == (float)0.0)
/* tgammaf pole */
@@ -29,7 +29,7 @@ __tgammal(long double x)
long double y = __ieee754_gammal_r(x,&local_signgam);
if(__glibc_unlikely (!isfinite (y) || y == 0)
- && (isfinite (x) || isinf (x) < 0)
+ && (isfinite (x) || (isinf (x) && x < 0.0))
&& _LIB_VERSION != _IEEE_) {
if(x==0.0)
return __kernel_standard_l(x,x,250); /* tgamma pole */
@@ -332,7 +332,6 @@ ___printf_fp (FILE *fp,
fpnum.ldbl = *(const long double *) args[0];
/* Check for special values: not a number or infinity. */
- int res;
if (isnan (fpnum.ldbl))
{
is_neg = signbit (fpnum.ldbl);
@@ -347,9 +346,9 @@ ___printf_fp (FILE *fp,
wspecial = L"nan";
}
}
- else if ((res = __isinfl (fpnum.ldbl)))
+ else if (isinf (fpnum.ldbl))
{
- is_neg = res < 0;
+ is_neg = signbit (fpnum.ldbl);
if (isupper (info->spec))
{
special = "INF";
@@ -377,11 +376,9 @@ ___printf_fp (FILE *fp,
fpnum.dbl = *(const double *) args[0];
/* Check for special values: not a number or infinity. */
- int res;
if (isnan (fpnum.dbl))
{
- union ieee754_double u = { .d = fpnum.dbl };
- is_neg = u.ieee.negative != 0;
+ is_neg = signbit (fpnum.dbl);
if (isupper (info->spec))
{
special = "NAN";
@@ -393,9 +390,9 @@ ___printf_fp (FILE *fp,
wspecial = L"nan";
}
}
- else if ((res = __isinf (fpnum.dbl)))
+ else if (isinf (fpnum.dbl))
{
- is_neg = res < 0;
+ is_neg = signbit (fpnum.dbl);
if (isupper (info->spec))
{
special = "INF";
@@ -180,7 +180,7 @@ __printf_fphex (FILE *fp,
}
else
{
- if (__isinfl (fpnum.ldbl))
+ if (isinf (fpnum.ldbl))
{
if (isupper (info->spec))
{
@@ -204,7 +204,6 @@ __printf_fphex (FILE *fp,
/* Check for special values: not a number or infinity. */
if (isnan (fpnum.dbl.d))
{
- negative = fpnum.dbl.ieee.negative != 0;
if (isupper (info->spec))
{
special = "NAN";
@@ -218,8 +217,7 @@ __printf_fphex (FILE *fp,
}
else
{
- int res = __isinf (fpnum.dbl.d);
- if (res)
+ if (isinf (fpnum.dbl.d))
{
if (isupper (info->spec))
{
@@ -231,11 +229,9 @@ __printf_fphex (FILE *fp,
special = "inf";
wspecial = L"inf";
}
- negative = res < 0;
}
- else
- negative = signbit (fpnum.dbl.d);
}
+ negative = signbit (fpnum.dbl.d);
}
if (special)
@@ -108,7 +108,7 @@ __printf_size (FILE *fp, const struct printf_info *info,
fpnum;
const void *ptr = &fpnum;
- int fpnum_sign = 0;
+ int is_neg = 0;
/* "NaN" or "Inf" for the special cases. */
const char *special = NULL;
@@ -117,7 +117,6 @@ __printf_size (FILE *fp, const struct printf_info *info,
struct printf_info fp_info;
int done = 0;
int wide = info->wide;
- int res;
/* Fetch the argument value. */
#ifndef __NO_LONG_DOUBLE_MATH
@@ -130,11 +129,11 @@ __printf_size (FILE *fp, const struct printf_info *info,
{
special = "nan";
wspecial = L"nan";
- // fpnum_sign = 0; Already zero
+ // is_neg = 0; Already zero
}
- else if ((res = __isinfl (fpnum.ldbl)))
+ else if (isinf (fpnum.ldbl))
{
- fpnum_sign = res;
+ is_neg = signbit (fpnum.ldbl);
special = "inf";
wspecial = L"inf";
}
@@ -155,11 +154,11 @@ __printf_size (FILE *fp, const struct printf_info *info,
{
special = "nan";
wspecial = L"nan";
- // fpnum_sign = 0; Already zero
+ // is_neg = 0; Already zero
}
- else if ((res = __isinf (fpnum.dbl.d)))
+ else if (isinf (fpnum.dbl.d))
{
- fpnum_sign = res;
+ is_neg = signbit (fpnum.dbl.d);
special = "inf";
wspecial = L"inf";
}
@@ -175,14 +174,14 @@ __printf_size (FILE *fp, const struct printf_info *info,
{
int width = info->prec > info->width ? info->prec : info->width;
- if (fpnum_sign < 0 || info->showsign || info->space)
+ if (is_neg || info->showsign || info->space)
--width;
width -= 3;
if (!info->left && width > 0)
PADN (' ', width);
- if (fpnum_sign < 0)
+ if (is_neg)
outchar ('-');
else if (info->showsign)
outchar ('+');