| Message ID | 000901cf7698$bf4a2e10$3dde8a30$@com |
|---|---|
| State | New |
| Headers | show |
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).
> 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
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)