This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] powerpc: fix check-before-set in SET_RESTORE_ROUND
- From: "Tulio Magno Quites Machado Filho" <tuliom at linux dot vnet dot ibm dot com>
- To: Paul Clarke <pc at us dot ibm dot com>, libc-alpha at sourceware dot org
- Cc:
- Date: Fri, 13 Oct 2017 17:16:40 -0300
- Subject: Re: [PATCH] powerpc: fix check-before-set in SET_RESTORE_ROUND
- Authentication-results: sourceware.org; auth=none
- References: <81a9460e-368d-997c-0e85-d7d8a0387e46@us.ibm.com>
Paul Clarke <pc@us.ibm.com> writes:
> A performance regression was introduced by commit
> 84d74e427a771906830800e574a72f8d25a954b8 "powerpc: Cleanup fenv_private.h".
>
> In the powerpc implementation of SET_RESTORE_ROUND, there is the
> following code in the "SET" function (slightly simplified):
> --
> old.fenv = fegetenv_register ();
>
> new.l = (old.l & _FPU_MASK_TRAPS_RN) | r; (1)
>
> if (new.l != old.l) (2)
> {
> if ((old.l & _FPU_ALL_TRAPS) != 0)
> (void) __fe_mask_env ();
> fesetenv_register (new.fenv); (3)
> --
>
> Line (1) sets the value of "new" to the current value of FPSCR,
> but masks off summary bits, exceptions, non-IEEE mode, and
> rounding mode, then ORs in the new rounding mode.
>
> Line (2) compares this new value to the current value in order to
> avoid setting a new value in the FPSCR (line (3)) unless something
> significant has changed (exception enables or rounding mode).
>
> The summary bits are not germane to the comparison, but are cleared
> in "new" and preserved in "old", resulting in false negative
> comparisons, and unnecessarily setting the FPSCR in those cases
> with associated negative performance impacts.
>
> The solution is to treat the summaries identically for "new" and "old":
> - save them in SET
> - leave them alone otherwise
> - restore the saved values in RESTORE
>
> Also minor changes:
> - expand _FPU_MASK_RN to 64bit hex, to match other MASKs
> - treat bit 52 (left-to-right) as reserved (since it is)
>
> 2017-10-10 Paul A. Clarke <pc@us.ibm.com>
>
> * sysdeps/powerpc/fpu/fenv_private.h: Fix masks to
> more properly handle summary bits.
I think this ChangeLog entry has to mention all the macros that are being
changed, e.g.:
* sysdeps/powerpc/fpu/fenv_private.h (_FPU_MASK_TRAPS_RN):
(_FPU_MASK_FRAC_INEX_RET_CC): Fix masks to more properly handle
summary bits.
(_FPU_MASK_RN): Expand _FPU_MASK_RN to 64bit hex.
(_FPU_MASK_NOT_RN_NI): Treat bit 52 (left-to-right) as reserved.
Reviewed-by: Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com>
--
Tulio Magno