[4/5] ARM: Cleanup fenv implementation
diff mbox

Message ID 003a01cf84cd$71cc24c0$55646e40$@com
State New
Headers show

Commit Message

Wilco June 10, 2014, 5 p.m. UTC
Hi,

This is a series of patches which improves the ARM fenv implementation. Improve rounding mode check
in libc_fesetround_vfp and libc_feholdsetround_vfp so it ignores out of range values.

ChangeLog:
2014-06-10  Wilco  <wdijkstr@arm.com>

	* sysdeps/arm/fenv_private.h (libc_fesetround_vfp)
	(libc_feholdsetround_vfp): Improve rounding mode check.
---
 sysdeps/arm/fenv_private.h |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Joseph Myers June 10, 2014, 5:25 p.m. UTC | #1
On Tue, 10 Jun 2014, Wilco wrote:

> This is a series of patches which improves the ARM fenv implementation. 
> Improve rounding mode check in libc_fesetround_vfp and 
> libc_feholdsetround_vfp so it ignores out of range values.

I don't see this as an improvement.  Invalid rounding modes should never 
get here, and it's confusing to change a variable called "round" to 
something other than the rounding mode.
Wilco June 11, 2014, 1:27 p.m. UTC | #2
Joseph wrote:
> On Tue, 10 Jun 2014, Wilco wrote:
> 
> > This is a series of patches which improves the ARM fenv implementation.
> > Improve rounding mode check in libc_fesetround_vfp and
> > libc_feholdsetround_vfp so it ignores out of range values.
> 
> I don't see this as an improvement.  Invalid rounding modes should never
> get here, and it's confusing to change a variable called "round" to
> something other than the rounding mode.

It's not only safer (for the unlikely case where you pass an incorrect
rounding mode) but also faster as it saves an instruction and enables the use
of CBZ. Note the AAarch64 version is exactly the same. 

Maybe naming it "round_adjust" or "round_diff" would make it clearer?

Wilco
Joseph Myers June 11, 2014, 5:13 p.m. UTC | #3
On Wed, 11 Jun 2014, Wilco wrote:

> Joseph wrote:
> > On Tue, 10 Jun 2014, Wilco wrote:
> > 
> > > This is a series of patches which improves the ARM fenv implementation.
> > > Improve rounding mode check in libc_fesetround_vfp and
> > > libc_feholdsetround_vfp so it ignores out of range values.
> > 
> > I don't see this as an improvement.  Invalid rounding modes should never
> > get here, and it's confusing to change a variable called "round" to
> > something other than the rounding mode.
> 
> It's not only safer (for the unlikely case where you pass an incorrect
> rounding mode) but also faster as it saves an instruction and enables the use
> of CBZ. Note the AAarch64 version is exactly the same. 

But this is an internal function that should never be able to receive an 
invalid rounding mode (if necessary, the caller, e.g. fesetround, should 
check), and the compiler should be able to see at each call site that only 
valid rounding modes can get there.  If the two don't generate the same 
code for some caller, then that suggests a microoptimization issue to be 
addressed in GCC.

> Maybe naming it "round_adjust" or "round_diff" would make it clearer?

Ordinary C usage would be "if (A != B)" not "if ((A ^ B) != 0)".  It's the 
compiler's job to convert the former into the latter if that's the best 
code sequence on a particular processor; the former is more idiomatic for 
humans, and gives the compiler just as much information, so is to be 
preferred.

Patch
diff mbox

diff --git a/sysdeps/arm/fenv_private.h b/sysdeps/arm/fenv_private.h
index 743df18..1d91fa6 100644
--- a/sysdeps/arm/fenv_private.h
+++ b/sysdeps/arm/fenv_private.h
@@ -42,9 +42,12 @@  libc_fesetround_vfp (int round)
 
   _FPU_GETCW (fpscr);
 
+  /* Check whether rounding modes are different.  */
+  round = (fpscr ^ round) & _FPU_MASK_RM;
+
   /* Set new rounding mode if different.  */
-  if (__glibc_unlikely ((fpscr & _FPU_MASK_RM) != round))
-    _FPU_SETCW ((fpscr & ~_FPU_MASK_RM) | round);
+  if (__glibc_unlikely (round != 0))
+    _FPU_SETCW (fpscr ^ round);
 }
 
 static __always_inline void
@@ -69,9 +72,12 @@  libc_feholdsetround_vfp (fenv_t *envp, int round)
   _FPU_GETCW (fpscr);
   envp->__cw = fpscr;
 
+  /* Check whether rounding modes are different.  */
+  round = (fpscr ^ round) & _FPU_MASK_RM;
+
   /* Set new rounding mode if different.  */
-  if (__glibc_unlikely ((fpscr & _FPU_MASK_RM) != round))
-    _FPU_SETCW ((fpscr & ~_FPU_MASK_RM) | round);
+  if (__glibc_unlikely (round != 0))
+    _FPU_SETCW (fpscr ^ round);
 }
 
 static __always_inline void