ARM - Rewrite feupdateenv
Commit Message
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
> 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
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.
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.)
... 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
> 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
"Wilco" <wdijkstr@arm.com> writes:
> 2014-05-23 Wilco <wdijkstr@arm.com>
Please use your full name.
Andreas.
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
@@ -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)