Re: Clean up powerpc fegetround / __fegetround inlines

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/30/2014 02:45 PM, Joseph Myers wrote:
> The natural fix for some linknamespace test failures, where C90 libm
> functions call C99 <fenv.h> functions, is to make fe* into weak
> aliases for __fe* and call __fe* from within libm as needed.
> 
> To do this, the __fe* names need to be available for that purpose -
> that is, they must not be used for something other than aliases of
> fe*.  On powerpc, however, __fegetround is an inline function in
> fenv_libc.h, with no corresponding fegetround inline function;
> fegetround has an equivalent macro expansion in bits/fenvinline.h, but
> that is disabled if __NO_MATH_INLINES (which is defined for building
> libm).
> 
> I see no need for that disabling; it's not even clear that
> __NO_MATH_INLINES should affect <fenv.h>, and the results of
> fegetround are completely defined so there is no semantic effect of
> that disabling at all outside glibc.  The x86 inline feraiseexcept is
> conditioned on __USE_EXTERN_INLINES not __NO_MATH_INLINES (but that's
> an inline function rather than a macro).
> 
> This patch removes the __NO_MATH_INLINES conditional on that
> fegetround macro, so resulting in it being expanded inline inside
> glibc.  In turn, this means that direct calls to __fegetround from C99
> functions in ldbl-128ibm can be changed to calls to fegetround, so
> that nofpu fenv_libc.h files don't need to define __fegetround at all
> and, by changing ldbl-128ibm files to use <fenv.h> not <fenv_libc.h>,
> non-e500 nofpu no longer needs an fenv_libc.h file.
> 
> The other macros in fenvinline.h are left conditional on
> __NO_MATH_INLINES, although since the only case where this should make
> a difference is one involving undefined behavior (if the argument to
> the function is not a valid exception macro).
> 
> The out-of-line definition for fegetround uses __fegetround (the
> inline function removed by this patch).  So this continues to work,
> the fenvinline.h header is made to define __fegetround, and then to
> define fegetround to call __fegetround.
> 
> Tested for powerpc32 (hard float) that installed stripped shared
> libraries are unchanged by this patch; also tested that powerpc-nofpu
> build still works.  (This patch does not itself fix any bugs; it
> simply cleans things up in preparation for separate bug fixes.)
> 
> 2014-12-30  Joseph Myers  <joseph@codesourcery.com>
> 
> 	* sysdeps/powerpc/bits/fenvinline.h (fegetround): Rename macro to
> 	__fegetround and redefine to call __fegetround.  Remove condition
> 	on [!__NO_MATH_INLINES].
> 	* sysdeps/powerpc/fpu/fenv_libc.h (__fegetround): Remove inline
> 	function.
> 	* sysdeps/powerpc/nofpu/fenv_libc.h: Remove file.
> 	* sysdeps/powerpc/powerpc32/e500/nofpu/fenv_libc.h (__fegetround):
> 	Remove macro.
> 	* sysdeps/ieee754/ldbl-128ibm/s_llrintl.c: Include <fenv.h>
> 	instead of <fenv_libc.h>.
> 	(__llrintl): Call fegetround instead of __fegetround.
> 	* sysdeps/ieee754/ldbl-128ibm/s_llroundl.c: Include <fenv.h>
> 	instead of <fenv_libc.h>.
> 	* sysdeps/ieee754/ldbl-128ibm/s_lrintl.c: Likewise.
> 	(__lrintl): Call fegetround instead of __fegetround.
> 	* sysdeps/ieee754/ldbl-128ibm/s_lroundl.c: Include <fenv.h>
> 	instead of <fenv_libc.h>.
> 	* sysdeps/ieee754/ldbl-128ibm/s_rintl.c: Likewise.
> 	(__rintl): Call fegetround instead of __fegetround.

Looks good to me.

Is there any reason you're not using [PATCH] in the prefix?

Is this a WIP or RFC?
 
> diff --git a/sysdeps/ieee754/ldbl-128ibm/s_llrintl.c b/sysdeps/ieee754/ldbl-128ibm/s_llrintl.c
> index 86996bf..3d9dee1 100644
> --- a/sysdeps/ieee754/ldbl-128ibm/s_llrintl.c
> +++ b/sysdeps/ieee754/ldbl-128ibm/s_llrintl.c
> @@ -18,7 +18,7 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <math.h>
> -#include <fenv_libc.h>
> +#include <fenv.h>
>  #include <math_ldbl_opt.h>
>  #include <float.h>
>  #include <ieee754.h>
> @@ -43,7 +43,7 @@ __llrintl (long double x)
>  #endif
>      )
>      {
> -      save_round = __fegetround ();
> +      save_round = fegetround ();
>  
>        if (__glibc_unlikely ((xh == -(double) (-__LONG_LONG_MAX__ - 1))))
>  	{
> diff --git a/sysdeps/ieee754/ldbl-128ibm/s_llroundl.c b/sysdeps/ieee754/ldbl-128ibm/s_llroundl.c
> index dd84074..98e4253 100644
> --- a/sysdeps/ieee754/ldbl-128ibm/s_llroundl.c
> +++ b/sysdeps/ieee754/ldbl-128ibm/s_llroundl.c
> @@ -18,7 +18,7 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <math.h>
> -#include <fenv_libc.h>
> +#include <fenv.h>
>  #include <math_ldbl_opt.h>
>  #include <float.h>
>  #include <ieee754.h>
> diff --git a/sysdeps/ieee754/ldbl-128ibm/s_lrintl.c b/sysdeps/ieee754/ldbl-128ibm/s_lrintl.c
> index 2b47c52..895a066 100644
> --- a/sysdeps/ieee754/ldbl-128ibm/s_lrintl.c
> +++ b/sysdeps/ieee754/ldbl-128ibm/s_lrintl.c
> @@ -18,7 +18,7 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <math.h>
> -#include <fenv_libc.h>
> +#include <fenv.h>
>  #include <math_ldbl_opt.h>
>  #include <float.h>
>  #include <ieee754.h>
> @@ -49,7 +49,7 @@ __lrintl (long double x)
>  #endif
>      )
>      {
> -      save_round = __fegetround ();
> +      save_round = fegetround ();
>  
>  #if __LONG_MAX__ == 2147483647
>        long long llhi = (long long) xh;
> diff --git a/sysdeps/ieee754/ldbl-128ibm/s_lroundl.c b/sysdeps/ieee754/ldbl-128ibm/s_lroundl.c
> index 27b72e2..6c3a314 100644
> --- a/sysdeps/ieee754/ldbl-128ibm/s_lroundl.c
> +++ b/sysdeps/ieee754/ldbl-128ibm/s_lroundl.c
> @@ -18,7 +18,7 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <math.h>
> -#include <fenv_libc.h>
> +#include <fenv.h>
>  #include <math_ldbl_opt.h>
>  #include <float.h>
>  #include <ieee754.h>
> diff --git a/sysdeps/ieee754/ldbl-128ibm/s_rintl.c b/sysdeps/ieee754/ldbl-128ibm/s_rintl.c
> index 20b75dd..f39e6fd 100644
> --- a/sysdeps/ieee754/ldbl-128ibm/s_rintl.c
> +++ b/sysdeps/ieee754/ldbl-128ibm/s_rintl.c
> @@ -21,7 +21,7 @@
>     when it's coded in C.  */
>  
>  #include <math.h>
> -#include <fenv_libc.h>
> +#include <fenv.h>
>  #include <math_ldbl_opt.h>
>  #include <float.h>
>  #include <ieee754.h>
> @@ -40,7 +40,7 @@ __rintl (long double x)
>  					     __builtin_inf ()), 1))
>      {
>        double orig_xh;
> -      int save_round = __fegetround ();
> +      int save_round = fegetround ();
>  
>        /* Long double arithmetic, including the canonicalisation below,
>  	 only works in round-to-nearest mode.  */
> diff --git a/sysdeps/powerpc/bits/fenvinline.h b/sysdeps/powerpc/bits/fenvinline.h
> index 00336f3..e914928 100644
> --- a/sysdeps/powerpc/bits/fenvinline.h
> +++ b/sysdeps/powerpc/bits/fenvinline.h
> @@ -16,23 +16,24 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#if (defined __GNUC__ && !defined _SOFT_FLOAT && !defined __NO_FPRS__	\
> -     && !defined __NO_MATH_INLINES)
> +#if defined __GNUC__ && !defined _SOFT_FLOAT && !defined __NO_FPRS__
>  
>  /* Inline definition for fegetround.  */
> -# define fegetround() \
> +# define __fegetround() \
>    (__extension__  ({ int __fegetround_result;				      \
>  		     __asm__ __volatile__				      \
>  		       ("mcrfs 7,7 ; mfcr %0"				      \
>  			: "=r"(__fegetround_result) : : "cr7");		      \
>  		     __fegetround_result & 3; }))
> +# define fegetround() __fegetround ()
>  
> +# ifndef __NO_MATH_INLINES
>  /* The weird 'i#*X' constraints on the following suppress a gcc
>     warning when __excepts is not a constant.  Otherwise, they mean the
>     same as just plain 'i'.  */
>  
>  /* Inline definition for feraiseexcept.  */
> -# define feraiseexcept(__excepts) \
> +#  define feraiseexcept(__excepts) \
>    ((__builtin_constant_p (__excepts)					      \
>      && ((__excepts) & ((__excepts)-1)) == 0				      \
>      && (__excepts) != FE_INVALID)					      \
> @@ -45,7 +46,7 @@
>     : (feraiseexcept) (__excepts))
>  
>  /* Inline definition for feclearexcept.  */
> -# define feclearexcept(__excepts) \
> +#  define feclearexcept(__excepts) \
>    ((__builtin_constant_p (__excepts)					      \
>      && ((__excepts) & ((__excepts)-1)) == 0				      \
>      && (__excepts) != FE_INVALID)					      \
> @@ -57,4 +58,6 @@
>        : 0)								      \
>     : (feclearexcept) (__excepts))
>  
> +# endif /* !__NO_MATH_INLINES.  */
> +
>  #endif /* __GNUC__ && !_SOFT_FLOAT && !__NO_FPRS__ */
> diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h
> index c1df5ce..125745e 100644
> --- a/sysdeps/powerpc/fpu/fenv_libc.h
> +++ b/sysdeps/powerpc/fpu/fenv_libc.h
> @@ -76,15 +76,6 @@ typedef union
>  
>  
>  static inline int
> -__fegetround (void)
> -{
> -  int result;
> -  asm volatile ("mcrfs 7,7\n\t"
> -		"mfcr  %0" : "=r"(result) : : "cr7");
> -  return result & 3;
> -}
> -
> -static inline int
>  __fesetround (int round)
>  {
>    if ((unsigned int) round < 2)
> diff --git a/sysdeps/powerpc/nofpu/fenv_libc.h b/sysdeps/powerpc/nofpu/fenv_libc.h
> deleted file mode 100644
> index dce1524..0000000
> --- a/sysdeps/powerpc/nofpu/fenv_libc.h
> +++ /dev/null
> @@ -1,31 +0,0 @@
> -/* Internal libc stuff for floating point environment routines.
> -   Copyright (C) 2007-2014 Free Software Foundation, Inc.
> -   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/>.  */
> -
> -#ifndef _FENV_LIBC_H
> -#define _FENV_LIBC_H	1
> -
> -/* fenv_libc.h is used in libm implementations of ldbl-128ibm.  So we
> -   need this version in the soft-fp to at minimum include fenv.h to
> -   get the fegetround definition.  */
> -
> -#include <fenv.h>
> -
> -/* ldbl-128ibm code uses __fegetround.  */
> -#define __fegetround()	fegetround ()
> -
> -#endif /* fenv_libc.h */
> diff --git a/sysdeps/powerpc/powerpc32/e500/nofpu/fenv_libc.h b/sysdeps/powerpc/powerpc32/e500/nofpu/fenv_libc.h
> index d8225e7..07eb675 100644
> --- a/sysdeps/powerpc/powerpc32/e500/nofpu/fenv_libc.h
> +++ b/sysdeps/powerpc/powerpc32/e500/nofpu/fenv_libc.h
> @@ -21,9 +21,6 @@
>  
>  #include <fenv.h>
>  
> -/* ldbl-128ibm code uses __fegetround.  */
> -#define __fegetround()	fegetround ()
> -
>  int __feraiseexcept_spe (int);
>  libm_hidden_proto (__feraiseexcept_spe)
>  
>