From patchwork Wed Aug 28 19:15:45 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Paul A. Clarke" X-Patchwork-Id: 34315 Received: (qmail 87970 invoked by alias); 28 Aug 2019 19:15:52 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 87960 invoked by uid 89); 28 Aug 2019 19:15:52 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-24.1 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 spammy= X-HELO: mx0a-001b2d01.pphosted.com Subject: Re: [PATCH] [powerpc] fe{en, dis}ableexcept optimize bit translations To: Adhemerval Zanella , libc-alpha@sourceware.org References: <1564802553-6645-1-git-send-email-pc@us.ibm.com> <1704adda-8688-25f0-3f1d-4d484da204a9@linaro.org> From: Paul Clarke Message-ID: <7c7ed15d-9dd5-2276-40a3-f29c168038db@us.ibm.com> Date: Wed, 28 Aug 2019 14:15:45 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <1704adda-8688-25f0-3f1d-4d484da204a9@linaro.org> 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 >> >> * 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 >> 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