ARM - Rewrite feupdateenv
diff mbox

Message ID 000901cf7698$bf4a2e10$3dde8a30$@com
State New
Headers show

Commit Message

Wilco May 23, 2014, 3:07 p.m. UTC
Hi,

This patch rewrites feupdateenv to improve performance by avoiding unnecessary FPSCR reads/writes
and to fix bug 16918 (https://sourceware.org/bugzilla/show_bug.cgi?id=16918).

OK?

Wilco

ChangeLog:
2014-05-23  Wilco  <wdijkstr@arm.com>

	* sysdeps/arm/feupdateenv.c (feupdateenv):
	Rewrite to reduce FPSCR accesses and fix bug 16918.
---
 sysdeps/arm/feupdateenv.c |   44 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 6 deletions(-)

Comments

Joseph Myers May 23, 2014, 3:23 p.m. UTC | #1
On Fri, 23 May 2014, Wilco wrote:

> This patch rewrites feupdateenv to improve performance by avoiding 
> unnecessary FPSCR reads/writes and to fix bug 16918 
> (https://sourceware.org/bugzilla/show_bug.cgi?id=16918).

It would be desirable to add an architecture-independent testcase that, if 
FE_NOMASK_ENV is defined and feupdateenv (FE_NOMASK_ENV) succeeds, the 
exceptions are then enabled as indicated by fegetexcept (or by getting 
SIGFPE, as test-fenv.c tests various cases, but a test using fegetexcept 
is simpler to write).  (It's best for this to be a new test rather than 
adding to the things in test-fenv.c.)

> 2014-05-23  Wilco  <wdijkstr@arm.com>

	[BZ #16918]

is how we indicate in a ChangeLog entry that a bug is being fixed (or 
partly fixed).
Wilco May 28, 2014, 1:10 p.m. UTC | #2
> From: Joseph Myers wrote:
> On Fri, 23 May 2014, Wilco wrote:
> 
> > This patch rewrites feupdateenv to improve performance by avoiding
> > unnecessary FPSCR reads/writes and to fix bug 16918
> > (https://sourceware.org/bugzilla/show_bug.cgi?id=16918).
> 
> It would be desirable to add an architecture-independent testcase that, if
> FE_NOMASK_ENV is defined and feupdateenv (FE_NOMASK_ENV) succeeds, the
> exceptions are then enabled as indicated by fegetexcept (or by getting
> SIGFPE, as test-fenv.c tests various cases, but a test using fegetexcept
> is simpler to write).  (It's best for this to be a new test rather than
> adding to the things in test-fenv.c.)

I'll add such a test in a separate patch (as a separate test it's best to 
test all of fesetenv, feupdateenv and feenableexcept so we've covered all 
of the special cases in one go).

Wilco
Joseph Myers June 10, 2014, 4:27 p.m. UTC | #3
On Fri, 23 May 2014, Wilco wrote:

> Hi,
> 
> This patch rewrites feupdateenv to improve performance by avoiding unnecessary FPSCR reads/writes
> and to fix bug 16918 (https://sourceware.org/bugzilla/show_bug.cgi?id=16918).
> 
> OK?

OK (with the [BZ #16918] notation and entry in the list of fixed bugs in 
NEWS, of course), if it passes the new test that is now checked in.
Joseph Myers June 24, 2014, 2:17 p.m. UTC | #4
On Tue, 10 Jun 2014, Joseph S. Myers wrote:

> On Fri, 23 May 2014, Wilco wrote:
> 
> > Hi,
> > 
> > This patch rewrites feupdateenv to improve performance by avoiding unnecessary FPSCR reads/writes
> > and to fix bug 16918 (https://sourceware.org/bugzilla/show_bug.cgi?id=16918).
> > 
> > OK?
> 
> OK (with the [BZ #16918] notation and entry in the list of fixed bugs in 
> NEWS, of course), if it passes the new test that is now checked in.

It appears this was committed without that NEWS update - please remember 
the entry in the list of fixed bugs in NEWS, and closing the bug in 
Bugzilla, when committing a patch that fixes a bug.  (It's also helpful if 
committers can update patchwork to mark their patches committed, to make 
it easier for reviewers to use patchwork to identify patches needing 
review.)
Joseph Myers June 24, 2014, 3:16 p.m. UTC | #5
... and your NEWS commit appears to have included bogus changes adding a 
list of patches to the NEWS file.  Please revert those changes.

+0001-Use-libc-calls.patch       0005-Remove-spaces.patch
+0002-Avoid-FPSCR-writes.patch   0006-Remove-include.patch
+0003-Improve-feupdateenv.patch  0007-Add-_FPU_MASK_RM.patch
+0004-Improve-fesetenv.patch     0008-Improve-rounding-mode-check.patch
Wilco June 24, 2014, 3:52 p.m. UTC | #6
> Joseph Myers wrote:
>
>  ... and your NEWS commit appears to have included bogus changes adding a
> list of patches to the NEWS file.  Please revert those changes.

All fixed now and I've closed BZ 16918.

Wilco
Andreas Schwab June 24, 2014, 3:57 p.m. UTC | #7
"Wilco" <wdijkstr@arm.com> writes:

> 2014-05-23  Wilco  <wdijkstr@arm.com>

Please use your full name.

Andreas.
Siddhesh Poyarekar June 26, 2014, 6:53 a.m. UTC | #8
On Tue, Jun 24, 2014 at 04:52:25PM +0100, Wilco wrote:
> > Joseph Myers wrote:
> >
> >  ... and your NEWS commit appears to have included bogus changes adding a
> > list of patches to the NEWS file.  Please revert those changes.
> 
> All fixed now and I've closed BZ 16918.

Please also sign up on patchwork.sourceware.org and clean up state for
your patches.  Additionally, please review the MAINTAINERS wiki page
and add yourself to it:

https://sourceware.org/glibc/wiki/MAINTAINERS

Thanks,
Siddhesh

Patch
diff mbox

diff --git a/sysdeps/arm/feupdateenv.c b/sysdeps/arm/feupdateenv.c
index 55a1502..d811678 100644
--- a/sysdeps/arm/feupdateenv.c
+++ b/sysdeps/arm/feupdateenv.c
@@ -18,26 +18,58 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #include <fenv.h>
-#include <fpu_control.h>
 #include <arm-features.h>
 
 
 int
 feupdateenv (const fenv_t *envp)
 {
-  fpu_control_t fpscr;
+  fpu_control_t fpscr, new_fpscr, updated_fpscr;
+  int excepts;
 
   /* Fail if a VFP unit isn't present.  */
   if (!ARM_HAVE_VFP)
     return 1;
 
   _FPU_GETCW (fpscr);
+  excepts = fpscr & FE_ALL_EXCEPT;
 
-  /* Install new environment.  */
-  fesetenv (envp);
+  if ((envp != FE_DFL_ENV) && (envp != FE_NOMASK_ENV))
+    {
+      /* Merge current exception flags with the saved fenv.  */
+      new_fpscr = envp->__cw | excepts;
+
+      /* Write new FPSCR if different (ignoring NZCV flags).  */
+      if (((fpscr ^ new_fpscr) & ~_FPU_MASK_NZCV) != 0)
+	_FPU_SETCW (new_fpscr);
+
+      /* Raise the exceptions if enabled in the new FP state.  */
+      if (excepts & (new_fpscr >> FE_EXCEPT_SHIFT))
+	return feraiseexcept (excepts);
+
+      return 0;
+    }
+
+  /* Preserve the reserved FPSCR flags.  */
+  new_fpscr = fpscr & (_FPU_RESERVED | FE_ALL_EXCEPT);
+  new_fpscr |= (envp == FE_DFL_ENV) ? _FPU_DEFAULT : _FPU_IEEE;
+
+  if (((new_fpscr ^ fpscr) & ~_FPU_MASK_NZCV) != 0)
+    {
+      _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 (updated_fpscr);
+
+      if (new_fpscr & ~updated_fpscr)
+	return 1;
+    }
+
+  /* Raise the exceptions if enabled in the new FP state.  */
+  if (excepts & (new_fpscr >> FE_EXCEPT_SHIFT))
+    return feraiseexcept (excepts);
 
-  /* Raise the saved exceptions.  */
-  feraiseexcept (fpscr & FE_ALL_EXCEPT);
   return 0;
 }
 libm_hidden_def (feupdateenv)