Re: [PATCH] [powerpc] fe{en,dis}ableexcept optimize bit translations

[Paul Clarke <pc at us dot ibm dot com>]

On 8/28/19 7:58 AM, Adhemerval Zanella wrote:
> On 03/08/2019 00:22, Paul A. Clarke wrote:
>> The exceptions passed to fe{en,dis}ableexcept() are defined in the ABI
>> as a bitmask, a combination of FE_INVALID, FE_OVERFLOW, etc.
>> Within the functions, these bits must be translated to/from the corresponding
>> enable bits in the Floating Point Status Control Register (FPSCR).
>> This translation is currently done bit-by-bit.  The compiler generates
>> a series of conditional bit operations.  Nicely, the "FE" exception
>> bits are all a uniform offset from the FPSCR enable bits, so the bit-by-bit
>> operation can instead be performed by a shift with appropriate masking.
>>
>> 2019-08-02  Paul A. Clarke  <pc@us.ibm.com>
>>
>> 	* sysdeps/powerpc/fpu/fenv_libc.h: Define FPSCR bitmasks.
>> 	(fenv_reg_to_exceptions): Replace bitwise operations with mask-shift.
>> 	(fenv_exceptions_to_reg): New.
>> 	* sysdeps/powerpc/fpu/fedisblxcpt.c (fedisableexcept): Replace bitwise
>> 	operation with call to fenv_exceptions_to_reg().
>> 	* sysdeps/powerpc/fpu/feenablxcpt.c (feenableexcept): Likewise.
>>
>> This patch is a prerequisite for the two patches I sent over the past two days:
>> - [powerpc] fe{en,dis}ableexcept, fesetmode: optimize FPSCR accesses
>> - [powerpc] SET_RESTORE_ROUND improvements
>> Apologies for sending these out-of-order.  I forgot about this one during the
>> freeze window.
> 
> LGTM with a suggestion below.
> 
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

>> diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h
>> index 9861f18..853239f 100644
>> --- a/sysdeps/powerpc/fpu/fenv_libc.h
>> +++ b/sysdeps/powerpc/fpu/fenv_libc.h
>> @@ -131,57 +131,103 @@ __fesetround_inline_nocheck (const int round)
>>  /* Definitions of all the FPSCR bit numbers */
>>  enum {
>>    FPSCR_FX = 0,    /* exception summary */
>> +#define FPSCR_FX_MASK (1 << (31 - FPSCR_FX))
>>    FPSCR_FEX,       /* enabled exception summary */
>> +#define FPSCR_FEX_MASK (1 << (31 - FPSCR_FEX))
[...]
>> +#define FPSCR_RN_MASK (FPSCR_RN_hi_MASK|FPSCR_RN_lo_MASK)
>> +#define FPSCR_ENABLES_MASK \
>> +  (FPSCR_VE_MASK|FPSCR_OE_MASK|FPSCR_UE_MASK|FPSCR_ZE_MASK|FPSCR_XE_MASK)
>> +#define FPSCR_BASIC_EXCEPTIONS_MASK \
>> +  (FPSCR_VX_MASK|FPSCR_OX_MASK|FPSCR_UX_MASK|FPSCR_ZX_MASK|FPSCR_XX_MASK)
>> +
>> +#define FPSCR_CONTROL_MASK (FPSCR_ENABLES_MASK|FPSCR_NI_MASK|FPSCR_RN_MASK)

> It is ok, but one suggestion I have is it would be simple to just define a
> macro to create the mask and use it.  Like:
> 
> #define FPSCR_MASK(bit) (1 << (31 - (bit)))
> 
> #define FPSCR_RN_MASK (FPSCR_MASK (FPSCR_RN_hi) | FPSCR_MASK (FPSCR_RN_lo))
> #define FPSCR_ENABLES_MASK \
>   (FPSCR_MASK(FPSCR_VE) | FPSCR_MASK (FPSCR_OE) | FPSCR_MASK (FPSCR_UE) \
>    | FPSCR_MASK (FPSCR_ZE) | FPSCR_MASK (FPSCR_XE))
> #define FPSCR_BASIC_EXCEPTIONS_MASK \
>   (FPSCR_MASK (FPSCR_VX) | FPSCR_MASK (FPSCR_OX) | FPSCR_MASK (FPSCR_UX) \
>    | FPSCR_MASK (FPSCR_ZX) | FPSCR_MASK (FPSCR_XX))

Thanks very much for the review, Adhemerval!

Apologies are due, as I forgot to include your "Reviewed-by" in the commit.  :-/

This is the essence of what I checked in.  I just used the newly suggested
FPSCR_MASK for all of the bits, and the more complex masks stayed the same.

diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h
index 9861f18..9956136 100644
--- a/sysdeps/powerpc/fpu/fenv_libc.h
+++ b/sysdeps/powerpc/fpu/fenv_libc.h
@@ -128,60 +128,108 @@ __fesetround_inline_nocheck (const int round)
   asm volatile ("mtfsfi 7,%0" : : "i" (round));
 }

+#define FPSCR_MASK(bit) (1 << (31 - (bit)))
+
 /* Definitions of all the FPSCR bit numbers */
 enum {
   FPSCR_FX = 0,    /* exception summary */
+#define FPSCR_FX_MASK (FPSCR_MASK (FPSCR_FX))
   FPSCR_FEX,       /* enabled exception summary */
+#define FPSCR_FEX_MASK (FPSCR_MASK FPSCR_FEX))
[...]
+#define FPSCR_RN_MASK (FPSCR_RN_hi_MASK|FPSCR_RN_lo_MASK)
+#define FPSCR_ENABLES_MASK \
+  (FPSCR_VE_MASK|FPSCR_OE_MASK|FPSCR_UE_MASK|FPSCR_ZE_MASK|FPSCR_XE_MASK)
+#define FPSCR_BASIC_EXCEPTIONS_MASK \
+  (FPSCR_VX_MASK|FPSCR_OX_MASK|FPSCR_UX_MASK|FPSCR_ZX_MASK|FPSCR_XX_MASK)
+
+#define FPSCR_CONTROL_MASK (FPSCR_ENABLES_MASK|FPSCR_NI_MASK|FPSCR_RN_MASK)

PC