[v2] Enable inlining issignalingf within glibc
diff mbox series

Message ID 1569851517-5682-1-git-send-email-pc@us.ibm.com
State New
Headers show
Series
  • [v2] Enable inlining issignalingf within glibc
Related show

Commit Message

Paul A. Clarke Sept. 30, 2019, 1:51 p.m. UTC
From: "Paul A. Clarke" <pc@us.ibm.com>

issignalingf is a very small function used in some areas where
better performance (and smaller code) might be helpful.

Create inline implementation for issignalingf.

2019-09-30  Paul A. Clarke  <pc@us.ibm.com>

	* sysdeps/generic/math_private.h (GET_FLOAT_WORD, SET_FLOAT_WORD):
	Moved...
	* include/math.h (GET_FLOAT_WORD, SET_FLOAT_WORD): to here.
	(__issignalingf): New, copied from
	sysdeps/iee754/flt-32/s_issignalingf.c.
---
v2:
- New approach as suggested by Joseph Myers: no implementations in
  math_private.h, instead in include/math.h.
- Moved GET_FLOAT_WORD from math_private.h to include/math.h in support.
- Brought along matching SET_FLOAT_WORD for consistency.

 include/math.h                 | 53 ++++++++++++++++++++++++++++++++++++++++++
 sysdeps/generic/math_private.h | 29 -----------------------
 2 files changed, 53 insertions(+), 29 deletions(-)

Comments

Paul A. Clarke Sept. 30, 2019, 3:19 p.m. UTC | #1
Sorry, this should've been noted as "v3".  ugh

On 9/30/19 8:51 AM, Paul A. Clarke wrote:
> From: "Paul A. Clarke" <pc@us.ibm.com>
> 
> issignalingf is a very small function used in some areas where
> better performance (and smaller code) might be helpful.
> 
> Create inline implementation for issignalingf.
> 
> 2019-09-30  Paul A. Clarke  <pc@us.ibm.com>
> 
> 	* sysdeps/generic/math_private.h (GET_FLOAT_WORD, SET_FLOAT_WORD):
> 	Moved...
> 	* include/math.h (GET_FLOAT_WORD, SET_FLOAT_WORD): to here.
> 	(__issignalingf): New, copied from
> 	sysdeps/iee754/flt-32/s_issignalingf.c.
> ---
> v2:
> - New approach as suggested by Joseph Myers: no implementations in
>   math_private.h, instead in include/math.h.
> - Moved GET_FLOAT_WORD from math_private.h to include/math.h in support.
> - Brought along matching SET_FLOAT_WORD for consistency.
> 
>  include/math.h                 | 53 ++++++++++++++++++++++++++++++++++++++++++
>  sysdeps/generic/math_private.h | 29 -----------------------
>  2 files changed, 53 insertions(+), 29 deletions(-)
> 
> diff --git a/include/math.h b/include/math.h
> index 79ebbae..a274f2b 100644
> --- a/include/math.h
> +++ b/include/math.h
> @@ -54,6 +54,59 @@ libm_hidden_proto (__expf128)
>  libm_hidden_proto (__expm1f128)
>  # endif
> 
> +#include <stdint.h>
> +#include <nan-high-order-bit.h>
> +
> +/* A union which permits us to convert between a float and a 32 bit
> +   int.  */
> +
> +typedef union
> +{
> +  float value;
> +  uint32_t word;
> +} ieee_float_shape_type;
> +
> +/* Get a 32 bit int from a float.  */
> +#ifndef GET_FLOAT_WORD
> +# define GET_FLOAT_WORD(i,d)					\
> +do {								\
> +  ieee_float_shape_type gf_u;					\
> +  gf_u.value = (d);						\
> +  (i) = gf_u.word;						\
> +} while (0)
> +#endif
> +
> +/* Set a float from a 32 bit int.  */
> +#ifndef SET_FLOAT_WORD
> +# define SET_FLOAT_WORD(d,i)					\
> +do {								\
> +  ieee_float_shape_type sf_u;					\
> +  sf_u.word = (i);						\
> +  (d) = sf_u.value;						\
> +} while (0)
> +#endif
> +
> +extern inline int
> +__issignalingf (float x)
> +{
> +  uint32_t xi;
> +  GET_FLOAT_WORD (xi, x);
> +#if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
> +  /* We only have to care about the high-order bit of x's significand, because
> +     having it set (sNaN) already makes the significand different from that
> +     used to designate infinity.  */
> +  return (xi & 0x7fc00000) == 0x7fc00000;
> +#else
> +  /* To keep the following comparison simple, toggle the quiet/signaling bit,
> +     so that it is set for sNaNs.  This is inverse to IEEE 754-2008 (as well as
> +     common practice for IEEE 754-1985).  */
> +  xi ^= 0x00400000;
> +  /* We have to compare for greater (instead of greater or equal), because x's
> +     significand being all-zero designates infinity not NaN.  */
> +  return (xi & 0x7fffffff) > 0x7fc00000;
> +#endif
> +}
> +
>  # if __HAVE_DISTINCT_FLOAT128
> 
>  /* __builtin_isinf_sign is broken in GCC < 7 for float128.  */
> diff --git a/sysdeps/generic/math_private.h b/sysdeps/generic/math_private.h
> index d91b929..9296324 100644
> --- a/sysdeps/generic/math_private.h
> +++ b/sysdeps/generic/math_private.h
> @@ -153,35 +153,6 @@ do {								\
>  } while (0)
>  #endif
> 
> -/* A union which permits us to convert between a float and a 32 bit
> -   int.  */
> -
> -typedef union
> -{
> -  float value;
> -  uint32_t word;
> -} ieee_float_shape_type;
> -
> -/* Get a 32 bit int from a float.  */
> -#ifndef GET_FLOAT_WORD
> -# define GET_FLOAT_WORD(i,d)					\
> -do {								\
> -  ieee_float_shape_type gf_u;					\
> -  gf_u.value = (d);						\
> -  (i) = gf_u.word;						\
> -} while (0)
> -#endif
> -
> -/* Set a float from a 32 bit int.  */
> -#ifndef SET_FLOAT_WORD
> -# define SET_FLOAT_WORD(d,i)					\
> -do {								\
> -  ieee_float_shape_type sf_u;					\
> -  sf_u.word = (i);						\
> -  (d) = sf_u.value;						\
> -} while (0)
> -#endif
> -
>  /* We need to guarantee an expansion of name when building
>     ldbl-128 files as another type (e.g _Float128).  */
>  #define mathx_hidden_def(name) hidden_def(name)
> 
PC
Paul A. Clarke Oct. 16, 2019, 4:50 p.m. UTC | #2
ping.

On 9/30/19 10:19 AM, Paul Clarke wrote:
> Sorry, this should've been noted as "v3".  ugh
> 
> On 9/30/19 8:51 AM, Paul A. Clarke wrote:
>> From: "Paul A. Clarke" <pc@us.ibm.com>
>>
>> issignalingf is a very small function used in some areas where
>> better performance (and smaller code) might be helpful.
>>
>> Create inline implementation for issignalingf.
>>
>> 2019-09-30  Paul A. Clarke  <pc@us.ibm.com>
>>
>> 	* sysdeps/generic/math_private.h (GET_FLOAT_WORD, SET_FLOAT_WORD):
>> 	Moved...
>> 	* include/math.h (GET_FLOAT_WORD, SET_FLOAT_WORD): to here.
>> 	(__issignalingf): New, copied from
>> 	sysdeps/iee754/flt-32/s_issignalingf.c.
>> ---
>> v2:
>> - New approach as suggested by Joseph Myers: no implementations in
>>   math_private.h, instead in include/math.h.
>> - Moved GET_FLOAT_WORD from math_private.h to include/math.h in support.
>> - Brought along matching SET_FLOAT_WORD for consistency.
>>
>>  include/math.h                 | 53 ++++++++++++++++++++++++++++++++++++++++++
>>  sysdeps/generic/math_private.h | 29 -----------------------
>>  2 files changed, 53 insertions(+), 29 deletions(-)
>>
>> diff --git a/include/math.h b/include/math.h
>> index 79ebbae..a274f2b 100644
>> --- a/include/math.h
>> +++ b/include/math.h
>> @@ -54,6 +54,59 @@ libm_hidden_proto (__expf128)
>>  libm_hidden_proto (__expm1f128)
>>  # endif
>>
>> +#include <stdint.h>
>> +#include <nan-high-order-bit.h>
>> +
>> +/* A union which permits us to convert between a float and a 32 bit
>> +   int.  */
>> +
>> +typedef union
>> +{
>> +  float value;
>> +  uint32_t word;
>> +} ieee_float_shape_type;
>> +
>> +/* Get a 32 bit int from a float.  */
>> +#ifndef GET_FLOAT_WORD
>> +# define GET_FLOAT_WORD(i,d)					\
>> +do {								\
>> +  ieee_float_shape_type gf_u;					\
>> +  gf_u.value = (d);						\
>> +  (i) = gf_u.word;						\
>> +} while (0)
>> +#endif
>> +
>> +/* Set a float from a 32 bit int.  */
>> +#ifndef SET_FLOAT_WORD
>> +# define SET_FLOAT_WORD(d,i)					\
>> +do {								\
>> +  ieee_float_shape_type sf_u;					\
>> +  sf_u.word = (i);						\
>> +  (d) = sf_u.value;						\
>> +} while (0)
>> +#endif
>> +
>> +extern inline int
>> +__issignalingf (float x)
>> +{
>> +  uint32_t xi;
>> +  GET_FLOAT_WORD (xi, x);
>> +#if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
>> +  /* We only have to care about the high-order bit of x's significand, because
>> +     having it set (sNaN) already makes the significand different from that
>> +     used to designate infinity.  */
>> +  return (xi & 0x7fc00000) == 0x7fc00000;
>> +#else
>> +  /* To keep the following comparison simple, toggle the quiet/signaling bit,
>> +     so that it is set for sNaNs.  This is inverse to IEEE 754-2008 (as well as
>> +     common practice for IEEE 754-1985).  */
>> +  xi ^= 0x00400000;
>> +  /* We have to compare for greater (instead of greater or equal), because x's
>> +     significand being all-zero designates infinity not NaN.  */
>> +  return (xi & 0x7fffffff) > 0x7fc00000;
>> +#endif
>> +}
>> +
>>  # if __HAVE_DISTINCT_FLOAT128
>>
>>  /* __builtin_isinf_sign is broken in GCC < 7 for float128.  */
>> diff --git a/sysdeps/generic/math_private.h b/sysdeps/generic/math_private.h
>> index d91b929..9296324 100644
>> --- a/sysdeps/generic/math_private.h
>> +++ b/sysdeps/generic/math_private.h
>> @@ -153,35 +153,6 @@ do {								\
>>  } while (0)
>>  #endif
>>
>> -/* A union which permits us to convert between a float and a 32 bit
>> -   int.  */
>> -
>> -typedef union
>> -{
>> -  float value;
>> -  uint32_t word;
>> -} ieee_float_shape_type;
>> -
>> -/* Get a 32 bit int from a float.  */
>> -#ifndef GET_FLOAT_WORD
>> -# define GET_FLOAT_WORD(i,d)					\
>> -do {								\
>> -  ieee_float_shape_type gf_u;					\
>> -  gf_u.value = (d);						\
>> -  (i) = gf_u.word;						\
>> -} while (0)
>> -#endif
>> -
>> -/* Set a float from a 32 bit int.  */
>> -#ifndef SET_FLOAT_WORD
>> -# define SET_FLOAT_WORD(d,i)					\
>> -do {								\
>> -  ieee_float_shape_type sf_u;					\
>> -  sf_u.word = (i);						\
>> -  (d) = sf_u.value;						\
>> -} while (0)
>> -#endif
>> -
>>  /* We need to guarantee an expansion of name when building
>>     ldbl-128 files as another type (e.g _Float128).  */
>>  #define mathx_hidden_def(name) hidden_def(name)
>>
> PC
>
Paul A. Clarke Oct. 28, 2019, 3:26 p.m. UTC | #3
ping #2.

On 10/16/19 11:50 AM, Paul Clarke wrote:
> ping.
> 
> On 9/30/19 10:19 AM, Paul Clarke wrote:
>> Sorry, this should've been noted as "v3".  ugh
>>
>> On 9/30/19 8:51 AM, Paul A. Clarke wrote:
>>> From: "Paul A. Clarke" <pc@us.ibm.com>
>>>
>>> issignalingf is a very small function used in some areas where
>>> better performance (and smaller code) might be helpful.
>>>
>>> Create inline implementation for issignalingf.
>>>
>>> 2019-09-30  Paul A. Clarke  <pc@us.ibm.com>
>>>
>>> 	* sysdeps/generic/math_private.h (GET_FLOAT_WORD, SET_FLOAT_WORD):
>>> 	Moved...
>>> 	* include/math.h (GET_FLOAT_WORD, SET_FLOAT_WORD): to here.
>>> 	(__issignalingf): New, copied from
>>> 	sysdeps/iee754/flt-32/s_issignalingf.c.
>>> ---
>>> v2:
>>> - New approach as suggested by Joseph Myers: no implementations in
>>>   math_private.h, instead in include/math.h.
>>> - Moved GET_FLOAT_WORD from math_private.h to include/math.h in support.
>>> - Brought along matching SET_FLOAT_WORD for consistency.
>>>
>>>  include/math.h                 | 53 ++++++++++++++++++++++++++++++++++++++++++
>>>  sysdeps/generic/math_private.h | 29 -----------------------
>>>  2 files changed, 53 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/include/math.h b/include/math.h
>>> index 79ebbae..a274f2b 100644
>>> --- a/include/math.h
>>> +++ b/include/math.h
>>> @@ -54,6 +54,59 @@ libm_hidden_proto (__expf128)
>>>  libm_hidden_proto (__expm1f128)
>>>  # endif
>>>
>>> +#include <stdint.h>
>>> +#include <nan-high-order-bit.h>
>>> +
>>> +/* A union which permits us to convert between a float and a 32 bit
>>> +   int.  */
>>> +
>>> +typedef union
>>> +{
>>> +  float value;
>>> +  uint32_t word;
>>> +} ieee_float_shape_type;
>>> +
>>> +/* Get a 32 bit int from a float.  */
>>> +#ifndef GET_FLOAT_WORD
>>> +# define GET_FLOAT_WORD(i,d)					\
>>> +do {								\
>>> +  ieee_float_shape_type gf_u;					\
>>> +  gf_u.value = (d);						\
>>> +  (i) = gf_u.word;						\
>>> +} while (0)
>>> +#endif
>>> +
>>> +/* Set a float from a 32 bit int.  */
>>> +#ifndef SET_FLOAT_WORD
>>> +# define SET_FLOAT_WORD(d,i)					\
>>> +do {								\
>>> +  ieee_float_shape_type sf_u;					\
>>> +  sf_u.word = (i);						\
>>> +  (d) = sf_u.value;						\
>>> +} while (0)
>>> +#endif
>>> +
>>> +extern inline int
>>> +__issignalingf (float x)
>>> +{
>>> +  uint32_t xi;
>>> +  GET_FLOAT_WORD (xi, x);
>>> +#if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
>>> +  /* We only have to care about the high-order bit of x's significand, because
>>> +     having it set (sNaN) already makes the significand different from that
>>> +     used to designate infinity.  */
>>> +  return (xi & 0x7fc00000) == 0x7fc00000;
>>> +#else
>>> +  /* To keep the following comparison simple, toggle the quiet/signaling bit,
>>> +     so that it is set for sNaNs.  This is inverse to IEEE 754-2008 (as well as
>>> +     common practice for IEEE 754-1985).  */
>>> +  xi ^= 0x00400000;
>>> +  /* We have to compare for greater (instead of greater or equal), because x's
>>> +     significand being all-zero designates infinity not NaN.  */
>>> +  return (xi & 0x7fffffff) > 0x7fc00000;
>>> +#endif
>>> +}
>>> +
>>>  # if __HAVE_DISTINCT_FLOAT128
>>>
>>>  /* __builtin_isinf_sign is broken in GCC < 7 for float128.  */
>>> diff --git a/sysdeps/generic/math_private.h b/sysdeps/generic/math_private.h
>>> index d91b929..9296324 100644
>>> --- a/sysdeps/generic/math_private.h
>>> +++ b/sysdeps/generic/math_private.h
>>> @@ -153,35 +153,6 @@ do {								\
>>>  } while (0)
>>>  #endif
>>>
>>> -/* A union which permits us to convert between a float and a 32 bit
>>> -   int.  */
>>> -
>>> -typedef union
>>> -{
>>> -  float value;
>>> -  uint32_t word;
>>> -} ieee_float_shape_type;
>>> -
>>> -/* Get a 32 bit int from a float.  */
>>> -#ifndef GET_FLOAT_WORD
>>> -# define GET_FLOAT_WORD(i,d)					\
>>> -do {								\
>>> -  ieee_float_shape_type gf_u;					\
>>> -  gf_u.value = (d);						\
>>> -  (i) = gf_u.word;						\
>>> -} while (0)
>>> -#endif
>>> -
>>> -/* Set a float from a 32 bit int.  */
>>> -#ifndef SET_FLOAT_WORD
>>> -# define SET_FLOAT_WORD(d,i)					\
>>> -do {								\
>>> -  ieee_float_shape_type sf_u;					\
>>> -  sf_u.word = (i);						\
>>> -  (d) = sf_u.value;						\
>>> -} while (0)
>>> -#endif
>>> -
>>>  /* We need to guarantee an expansion of name when building
>>>     ldbl-128 files as another type (e.g _Float128).  */
>>>  #define mathx_hidden_def(name) hidden_def(name)
>>>
>> PC
>>
Joseph Myers Oct. 29, 2019, 10:18 p.m. UTC | #4
On Mon, 30 Sep 2019, Paul A. Clarke wrote:

> From: "Paul A. Clarke" <pc@us.ibm.com>
> 
> issignalingf is a very small function used in some areas where
> better performance (and smaller code) might be helpful.
> 
> Create inline implementation for issignalingf.
> 
> 2019-09-30  Paul A. Clarke  <pc@us.ibm.com>
> 
> 	* sysdeps/generic/math_private.h (GET_FLOAT_WORD, SET_FLOAT_WORD):
> 	Moved...
> 	* include/math.h (GET_FLOAT_WORD, SET_FLOAT_WORD): to here.
> 	(__issignalingf): New, copied from
> 	sysdeps/iee754/flt-32/s_issignalingf.c.

This is OK.

There's the question of what to do about issignalingf_inline in 
sysdeps/ieee754/flt-32/math_config.h and its uses in 
sysdeps/ieee754/flt-32/e_powf.c.  Arguably they ought to move to just 
calling issignaling.  If however the slightly different code in 
issignalingf_inline results in better code in the callers (which would 
need to be checked, I don't know whether masking or shifting would be 
better in general), that would indicate (a) changing the code in the 
generic inline and (b) filing a GCC bug report to convert one into the 
other as an optimization.
Paul A. Clarke Oct. 31, 2019, 9:14 p.m. UTC | #5
On 10/29/19 5:18 PM, Joseph Myers wrote:
> On Mon, 30 Sep 2019, Paul A. Clarke wrote:
>> issignalingf is a very small function used in some areas where
>> better performance (and smaller code) might be helpful.
>>
>> Create inline implementation for issignalingf.
>>
>> 2019-09-30  Paul A. Clarke  <pc@us.ibm.com>
>>
>> 	* sysdeps/generic/math_private.h (GET_FLOAT_WORD, SET_FLOAT_WORD):
>> 	Moved...
>> 	* include/math.h (GET_FLOAT_WORD, SET_FLOAT_WORD): to here.
>> 	(__issignalingf): New, copied from
>> 	sysdeps/iee754/flt-32/s_issignalingf.c.
> 
> This is OK.
> 
> There's the question of what to do about issignalingf_inline in 
> sysdeps/ieee754/flt-32/math_config.h and its uses in 
> sysdeps/ieee754/flt-32/e_powf.c.  Arguably they ought to move to just 
> calling issignaling.  If however the slightly different code in 
> issignalingf_inline results in better code in the callers (which would 
> need to be checked, I don't know whether masking or shifting would be 
> better in general), that would indicate (a) changing the code in the 
> generic inline and (b) filing a GCC bug report to convert one into the 
> other as an optimization.
> 

The generated code was identical on ppc64le.  There were a few minor differences on x86_64 (GCC7 and GCC8; looking at just the mnemonics; '-' is current issignalingf_inline, '+' is new __issignalingf):
--
@@ -210,7 +210,7 @@
 xor    
 movss  
 
-add    
+and    
 cmp    
 jbe    
 mov    
@@ -244,10 +244,9 @@
 movaps 
 retq   
 xor    
-lea    
+and    
 cmp    
 ja     
 movss  
---

...but I am far from an expert as to which is better on x86_64.

PC

Patch
diff mbox series

diff --git a/include/math.h b/include/math.h
index 79ebbae..a274f2b 100644
--- a/include/math.h
+++ b/include/math.h
@@ -54,6 +54,59 @@  libm_hidden_proto (__expf128)
 libm_hidden_proto (__expm1f128)
 # endif
 
+#include <stdint.h>
+#include <nan-high-order-bit.h>
+
+/* A union which permits us to convert between a float and a 32 bit
+   int.  */
+
+typedef union
+{
+  float value;
+  uint32_t word;
+} ieee_float_shape_type;
+
+/* Get a 32 bit int from a float.  */
+#ifndef GET_FLOAT_WORD
+# define GET_FLOAT_WORD(i,d)					\
+do {								\
+  ieee_float_shape_type gf_u;					\
+  gf_u.value = (d);						\
+  (i) = gf_u.word;						\
+} while (0)
+#endif
+
+/* Set a float from a 32 bit int.  */
+#ifndef SET_FLOAT_WORD
+# define SET_FLOAT_WORD(d,i)					\
+do {								\
+  ieee_float_shape_type sf_u;					\
+  sf_u.word = (i);						\
+  (d) = sf_u.value;						\
+} while (0)
+#endif
+
+extern inline int
+__issignalingf (float x)
+{
+  uint32_t xi;
+  GET_FLOAT_WORD (xi, x);
+#if HIGH_ORDER_BIT_IS_SET_FOR_SNAN
+  /* We only have to care about the high-order bit of x's significand, because
+     having it set (sNaN) already makes the significand different from that
+     used to designate infinity.  */
+  return (xi & 0x7fc00000) == 0x7fc00000;
+#else
+  /* To keep the following comparison simple, toggle the quiet/signaling bit,
+     so that it is set for sNaNs.  This is inverse to IEEE 754-2008 (as well as
+     common practice for IEEE 754-1985).  */
+  xi ^= 0x00400000;
+  /* We have to compare for greater (instead of greater or equal), because x's
+     significand being all-zero designates infinity not NaN.  */
+  return (xi & 0x7fffffff) > 0x7fc00000;
+#endif
+}
+
 # if __HAVE_DISTINCT_FLOAT128
 
 /* __builtin_isinf_sign is broken in GCC < 7 for float128.  */
diff --git a/sysdeps/generic/math_private.h b/sysdeps/generic/math_private.h
index d91b929..9296324 100644
--- a/sysdeps/generic/math_private.h
+++ b/sysdeps/generic/math_private.h
@@ -153,35 +153,6 @@  do {								\
 } while (0)
 #endif
 
-/* A union which permits us to convert between a float and a 32 bit
-   int.  */
-
-typedef union
-{
-  float value;
-  uint32_t word;
-} ieee_float_shape_type;
-
-/* Get a 32 bit int from a float.  */
-#ifndef GET_FLOAT_WORD
-# define GET_FLOAT_WORD(i,d)					\
-do {								\
-  ieee_float_shape_type gf_u;					\
-  gf_u.value = (d);						\
-  (i) = gf_u.word;						\
-} while (0)
-#endif
-
-/* Set a float from a 32 bit int.  */
-#ifndef SET_FLOAT_WORD
-# define SET_FLOAT_WORD(d,i)					\
-do {								\
-  ieee_float_shape_type sf_u;					\
-  sf_u.word = (i);						\
-  (d) = sf_u.value;						\
-} while (0)
-#endif
-
 /* We need to guarantee an expansion of name when building
    ldbl-128 files as another type (e.g _Float128).  */
 #define mathx_hidden_def(name) hidden_def(name)