[2/2] ARM: Improve fenv implementation

Message ID 001201cf6074$104601d0$30d20570$@com
State Committed
Headers

Commit Message

Wilco Dijkstra April 25, 2014, 10:49 a.m. UTC
  Hi,

The second patch improves the fenv implementation by using the inline functions from fenv_private
where possible rather than duplicating functionality. The remaining functions are optimized in a
similar way to avoid unnecessary FPSCR writes (but no longer use __glibc_unlikely like in the
previous version).

OK for commit?

Wilco
---
 sysdeps/arm/fclrexcpt.c    |   10 +++++-----
 sysdeps/arm/fedisblxcpt.c  |    4 +++-
 sysdeps/arm/feenablxcpt.c  |   10 +++++-----
 sysdeps/arm/fegetround.c   |   14 ++------------
 sysdeps/arm/feholdexcpt.c  |   16 ++--------------
 sysdeps/arm/fesetenv.c     |   42 +++++++++++++++++++++---------------------
 sysdeps/arm/fesetround.c   |   10 ++--------
 sysdeps/arm/feupdateenv.c  |   24 ++++++++++++++++--------
 sysdeps/arm/fgetexcptflg.c |    9 ++-------
 sysdeps/arm/fsetexcptflg.c |   13 ++++++++-----
 sysdeps/arm/ftestexcept.c  |   11 +++--------
 sysdeps/arm/setfpucw.c     |   11 ++++++-----
 12 files changed, 75 insertions(+), 99 deletions(-)
  

Comments

Joseph Myers May 6, 2014, 10:39 p.m. UTC | #1
On Fri, 25 Apr 2014, Wilco wrote:

> The second patch improves the fenv implementation by using the inline 
> functions from fenv_private where possible rather than duplicating 
> functionality. The remaining functions are optimized in a similar way to 
> avoid unnecessary FPSCR writes (but no longer use __glibc_unlikely like 
> in the previous version).
> 
> OK for commit?

OK, but I think further rearrangement in feupdateenv will end up being 
needed to fix bug 16918 (libc_feupdateenv_vfp returns void and doesn't 
test whether exception traps were successfully enabled, so the approach in 
your implementation is not well-suited to handling the FE_NOMASK_ENV case 
properly, although it matches the buggy handling in the existing code).
  
Wilco Dijkstra May 7, 2014, 8:15 p.m. UTC | #2
> Joseph Myers wrote:
> On Fri, 25 Apr 2014, Wilco wrote:
> 
> > The second patch improves the fenv implementation by using the inline
> > functions from fenv_private where possible rather than duplicating
> > functionality. The remaining functions are optimized in a similar way to
> > avoid unnecessary FPSCR writes (but no longer use __glibc_unlikely like
> > in the previous version).
> >
> > OK for commit?
> 
> OK, but I think further rearrangement in feupdateenv will end up being
> needed to fix bug 16918 (libc_feupdateenv_vfp returns void and doesn't
> test whether exception traps were successfully enabled, so the approach in
> your implementation is not well-suited to handling the FE_NOMASK_ENV case
> properly, although it matches the buggy handling in the existing code).

Alright, since I have some further fenv changes in flight, I'll have a go at
fixing that as well.

Wilco
  

Patch

diff --git a/sysdeps/arm/fclrexcpt.c b/sysdeps/arm/fclrexcpt.c
index 098a9bb..824145e 100644
--- a/sysdeps/arm/fclrexcpt.c
+++ b/sysdeps/arm/fclrexcpt.c
@@ -24,7 +24,7 @@ 
 int
 __feclearexcept (int excepts)
 {
-  fpu_control_t fpscr;
+  fpu_control_t fpscr, new_fpscr;
 
   /* Fail if a VFP unit isn't present unless nothing needs to be done.  */
   if (!ARM_HAVE_VFP)
@@ -32,11 +32,11 @@  __feclearexcept (int excepts)
 
   _FPU_GETCW (fpscr);
   excepts &= FE_ALL_EXCEPT;
+  new_fpscr = fpscr & ~excepts;
 
-  /* Clear the relevant bits.  */
-  fpscr = (fpscr & ~FE_ALL_EXCEPT) | (fpscr & FE_ALL_EXCEPT & ~excepts);
-
-  _FPU_SETCW (fpscr);
+  /* Write new exception flags if changed.  */
+  if (new_fpscr != fpscr)
+    _FPU_SETCW (new_fpscr);
 
   return 0;
 }
diff --git a/sysdeps/arm/fedisblxcpt.c b/sysdeps/arm/fedisblxcpt.c
index f2956cd..d5e0f00 100644
--- a/sysdeps/arm/fedisblxcpt.c
+++ b/sysdeps/arm/fedisblxcpt.c
@@ -35,7 +35,9 @@  fedisableexcept (int excepts)
   excepts &= FE_ALL_EXCEPT;
   new_fpscr = fpscr & ~(excepts << FE_EXCEPT_SHIFT);
 
-  _FPU_SETCW (new_fpscr);
+  /* Write new exceptions if changed.  */
+  if (new_fpscr != fpscr)
+    _FPU_SETCW (new_fpscr);
 
   return (fpscr >> FE_EXCEPT_SHIFT) & FE_ALL_EXCEPT;
 }
diff --git a/sysdeps/arm/feenablxcpt.c b/sysdeps/arm/feenablxcpt.c
index afd8943..e649b2f 100644
--- a/sysdeps/arm/feenablxcpt.c
+++ b/sysdeps/arm/feenablxcpt.c
@@ -35,15 +35,15 @@  feenableexcept (int excepts)
   excepts &= FE_ALL_EXCEPT;
   new_fpscr = fpscr | (excepts << FE_EXCEPT_SHIFT);
 
-  _FPU_SETCW (new_fpscr);
-
-  if (excepts != 0)
+  if (new_fpscr != fpscr)
     {
+      _FPU_SETCW (new_fpscr);
+
       /* Not all VFP architectures support trapping exceptions, so
 	 test whether the relevant bits were set and fail if not.  */
       _FPU_GETCW (new_fpscr);
-      if ((new_fpscr & (excepts << FE_EXCEPT_SHIFT))
-	  != (excepts << FE_EXCEPT_SHIFT))
+
+      if (((new_fpscr >> FE_EXCEPT_SHIFT) & excepts) != excepts)
 	return -1;
     }
 
diff --git a/sysdeps/arm/fegetround.c b/sysdeps/arm/fegetround.c
index 1c9c151..fbad0b3 100644
--- a/sysdeps/arm/fegetround.c
+++ b/sysdeps/arm/fegetround.c
@@ -16,22 +16,12 @@ 
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <fenv.h>
-#include <fpu_control.h>
-#include <arm-features.h>
+#include <get-rounding-mode.h>
 
 
 int
 fegetround (void)
 {
-  fpu_control_t fpscr;
-
-  /* FE_TONEAREST is the only supported rounding mode
-     if a VFP unit isn't present.  */
-  if (!ARM_HAVE_VFP)
-    return FE_TONEAREST;
-
-  _FPU_GETCW (fpscr);
-  return fpscr & FE_TOWARDZERO;
+  return get_rounding_mode ();
 }
 libm_hidden_def (fegetround)
diff --git a/sysdeps/arm/feholdexcpt.c b/sysdeps/arm/feholdexcpt.c
index 258ba66..2d79e0c 100644
--- a/sysdeps/arm/feholdexcpt.c
+++ b/sysdeps/arm/feholdexcpt.c
@@ -16,30 +16,18 @@ 
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <fenv.h>
-#include <fpu_control.h>
+#include <fenv_private.h>
 #include <arm-features.h>
 
 
 int
 feholdexcept (fenv_t *envp)
 {
-  fpu_control_t fpscr;
-
   /* Fail if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return 1;
 
-  _FPU_GETCW (fpscr);
-  envp->__cw = fpscr;
-
-  /* Now set all exceptions to non-stop.  */
-  fpscr &= ~(FE_ALL_EXCEPT << FE_EXCEPT_SHIFT);
-
-  /* And clear all exception flags.  */
-  fpscr &= ~FE_ALL_EXCEPT;
-
-  _FPU_SETCW (fpscr);
+  libc_feholdexcept_vfp (envp);
   return 0;
 }
 
diff --git a/sysdeps/arm/fesetenv.c b/sysdeps/arm/fesetenv.c
index 43b9b47..8677227 100644
--- a/sysdeps/arm/fesetenv.c
+++ b/sysdeps/arm/fesetenv.c
@@ -16,43 +16,43 @@ 
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <fenv.h>
-#include <fpu_control.h>
+#include <fenv_private.h>
 #include <arm-features.h>
 
 
 int
 __fesetenv (const fenv_t *envp)
 {
-  fpu_control_t fpscr;
-
   /* Fail if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return 1;
 
-  _FPU_GETCW (fpscr);
+  if ((envp == FE_DFL_ENV) || (envp == FE_NOMASK_ENV))
+    {
+      fpu_control_t fpscr, new_fpscr;
+
+      _FPU_GETCW (fpscr);
 
-  /* Preserve the reserved FPSCR flags.  */
-  fpscr &= _FPU_RESERVED;
+      /* Preserve the reserved FPSCR flags.  */
+      new_fpscr = fpscr & _FPU_RESERVED;
 
-  if (envp == FE_DFL_ENV)
-    fpscr |= _FPU_DEFAULT;
-  else if (envp == FE_NOMASK_ENV)
-    fpscr |= _FPU_IEEE;
-  else
-    fpscr |= envp->__cw & ~_FPU_RESERVED;
+      if (envp == FE_DFL_ENV)
+	_FPU_SETCW (new_fpscr | _FPU_DEFAULT);
+      else
+	{
+	  _FPU_SETCW (new_fpscr | _FPU_IEEE);
+	  /* Not all VFP architectures support trapping exceptions, so
+	     test whether the relevant bits were set and fail if not.  */
+	  _FPU_GETCW (fpscr);
 
-  _FPU_SETCW (fpscr);
+	  if ((fpscr & _FPU_IEEE) != _FPU_IEEE)
+	    return 1;
+	}
 
-  if (envp == FE_NOMASK_ENV)
-    {
-      /* Not all VFP architectures support trapping exceptions, so
-	 test whether the relevant bits were set and fail if not.  */
-      _FPU_GETCW (fpscr);
-      if ((fpscr & _FPU_IEEE) != _FPU_IEEE)
-	return 1;
+      return 0;
     }
 
+  libc_fesetenv_vfp (envp);
   return 0;
 }
 
diff --git a/sysdeps/arm/fesetround.c b/sysdeps/arm/fesetround.c
index d1b92dc..f52c50a 100644
--- a/sysdeps/arm/fesetround.c
+++ b/sysdeps/arm/fesetround.c
@@ -16,28 +16,22 @@ 
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <fenv.h>
-#include <fpu_control.h>
+#include <fenv_private.h>
 #include <arm-features.h>
 
 
 int
 fesetround (int round)
 {
-  fpu_control_t fpscr;
-
   /* FE_TONEAREST is the only supported rounding mode
      if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return (round == FE_TONEAREST) ? 0 : 1;
 
-  /* Fail if the rounding mode is not valid.  */
   if (round & ~FE_TOWARDZERO)
     return 1;
 
-  _FPU_GETCW (fpscr);
-  fpscr = (fpscr & ~FE_TOWARDZERO) | round;
-  _FPU_SETCW (fpscr);
+  libc_fesetround_vfp (round);
   return 0;
 }
 
diff --git a/sysdeps/arm/feupdateenv.c b/sysdeps/arm/feupdateenv.c
index 7cf6206..4b2cf0f 100644
--- a/sysdeps/arm/feupdateenv.c
+++ b/sysdeps/arm/feupdateenv.c
@@ -17,27 +17,35 @@ 
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <fenv.h>
-#include <fpu_control.h>
+#include <fenv_private.h>
 #include <arm-features.h>
 
 
 int
 __feupdateenv (const fenv_t *envp)
 {
-  fpu_control_t fpscr;
+  fenv_t fenv;
 
   /* Fail if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return 1;
 
-  _FPU_GETCW (fpscr);
+  if ((envp == FE_DFL_ENV) || (envp == FE_NOMASK_ENV))
+    {
+      fpu_control_t fpscr;
 
-  /* Install new environment.  */
-  fesetenv (envp);
+      _FPU_GETCW (fpscr);
 
-  /* Raise the saved exceptions.  */
-  feraiseexcept (fpscr & FE_ALL_EXCEPT);
+      /* Preserve the reserved FPSCR flags.  */
+      fpscr &= _FPU_RESERVED;
+      fpscr |= (envp == FE_DFL_ENV) ? _FPU_DEFAULT : _FPU_IEEE;
+
+      /* Create a valid fenv to pass to libc_feupdateenv_vfp.  */
+      fenv.__cw = fpscr;
+      envp = &fenv;
+    }
+
+  libc_feupdateenv_vfp (envp);
   return 0;
 }
 
diff --git a/sysdeps/arm/fgetexcptflg.c b/sysdeps/arm/fgetexcptflg.c
index e1eb420..19c8b6d 100644
--- a/sysdeps/arm/fgetexcptflg.c
+++ b/sysdeps/arm/fgetexcptflg.c
@@ -17,23 +17,18 @@ 
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <fenv.h>
-#include <fpu_control.h>
+#include <fenv_private.h>
 #include <arm-features.h>
 
 
 int
 __fegetexceptflag (fexcept_t *flagp, int excepts)
 {
-  fpu_control_t fpscr;
-
   /* Fail if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return 1;
 
-  _FPU_GETCW (fpscr);
-
-  *flagp = fpscr & excepts & FE_ALL_EXCEPT;
+  *flagp = libc_fetestexcept_vfp (excepts);
   return 0;
 }
 
diff --git a/sysdeps/arm/fsetexcptflg.c b/sysdeps/arm/fsetexcptflg.c
index a594e15..c1a6c1a 100644
--- a/sysdeps/arm/fsetexcptflg.c
+++ b/sysdeps/arm/fsetexcptflg.c
@@ -25,20 +25,23 @@ 
 int
 __fesetexceptflag (const fexcept_t *flagp, int excepts)
 {
-  fpu_control_t fpscr;
+  fpu_control_t fpscr, new_fpscr;
 
   /* Fail if a VFP unit isn't present unless nothing needs to be done.  */
   if (!ARM_HAVE_VFP)
     return (excepts != 0);
 
   _FPU_GETCW (fpscr);
+  excepts &= FE_ALL_EXCEPT;
 
   /* Set the desired exception mask.  */
-  fpscr &= ~(excepts & FE_ALL_EXCEPT);
-  fpscr |= (*flagp & excepts & FE_ALL_EXCEPT);
+  new_fpscr = fpscr & ~excepts;
+  new_fpscr |= *flagp & excepts;
+
+  /* Write new exception flags if changed.  */
+  if (new_fpscr != fpscr)
+    _FPU_SETCW (new_fpscr);
 
-  /* Save state back to the FPU.  */
-  _FPU_SETCW (fpscr);
   return 0;
 }
 
diff --git a/sysdeps/arm/ftestexcept.c b/sysdeps/arm/ftestexcept.c
index de082b2..6c5d3a8 100644
--- a/sysdeps/arm/ftestexcept.c
+++ b/sysdeps/arm/ftestexcept.c
@@ -16,23 +16,18 @@ 
    License along with the GNU C Library.  If not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <fenv.h>
-#include <fpu_control.h>
+#include <fenv_private.h>
 #include <arm-features.h>
 
 
 int
 fetestexcept (int excepts)
 {
-  fpu_control_t fpscr;
-
   /* Return no exception flags if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return 0;
 
-  /* Get current exceptions.  */
-  _FPU_GETCW (fpscr);
-
-  return fpscr & excepts & FE_ALL_EXCEPT;
+  return libc_fetestexcept_vfp (excepts);
 }
+
 libm_hidden_def (fetestexcept)
diff --git a/sysdeps/arm/setfpucw.c b/sysdeps/arm/setfpucw.c
index 7416377..259b020 100644
--- a/sysdeps/arm/setfpucw.c
+++ b/sysdeps/arm/setfpucw.c
@@ -24,19 +24,20 @@ 
 void
 __setfpucw (fpu_control_t set)
 {
-  fpu_control_t fpscr;
+  fpu_control_t fpscr, new_fpscr;
 
   /* Do nothing if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return;
 
-  /* Fetch the current control word.  */
   _FPU_GETCW (fpscr);
 
   /* Preserve the reserved bits, and set the rest as the user
      specified (or the default, if the user gave zero).  */
-  fpscr &= _FPU_RESERVED;
-  fpscr |= set & ~_FPU_RESERVED;
+  new_fpscr = fpscr & _FPU_RESERVED;
+  new_fpscr |= set & ~_FPU_RESERVED;
 
-  _FPU_SETCW (fpscr);
+  /* Write FPSCR if changed.  */
+  if (new_fpscr != fpscr)
+    _FPU_SETCW (fpscr);
 }