diff mbox series

[v2,4/6,powerpc] libc_feholdsetround_noex_ppc_ctx: optimize FPSCR write

Message ID 1568918810-20393-5-git-send-email-pc@us.ibm.com
State New
Headers show
Series Various FPSCR-related changes | expand

Commit Message

Paul A. Clarke Sept. 19, 2019, 6:46 p.m. UTC
From: "Paul A. Clarke" <pc@us.ibm.com>

libc_feholdsetround_noex_ppc_ctx currently does, basically:
1. Read FPSCR, save to context.
2. Create new FPSCR value: clear enables and set new rounding mode.
3. Write new value to FPSCR.

Since other bits just pass through, there is no need to write them.

Instead, write just the changed values (enables and rounding mode),
which can be a bit more efficient.

2019-09-19  Paul A. Clarke  <pc@us.ibm.com>

	* sysdeps/powerpc/fpu/fenv_private.h
	(libc_feholdsetround_noex_ppc_ctx): Call fesetenv_mode instead
	of fesetenv_register.
---
v2: No change.

 sysdeps/powerpc/fpu/fenv_private.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Paul E Murphy Sept. 23, 2019, 3:54 p.m. UTC | #1
On 9/19/19 1:46 PM, Paul A. Clarke wrote:
> From: "Paul A. Clarke" <pc@us.ibm.com>
> 
> libc_feholdsetround_noex_ppc_ctx currently does, basically:
The listing reads awkwardly for me. I suggest a little cleanup. There is 
no need to post a new patch against my suggestion.

> 1. Read FPSCR, save to context.
> 2. Create new FPSCR value: clear enables and set new rounding mode.
> 3. Write new value to FPSCR.
> 
> Since other bits just pass through, there is no need to write them.
> 
> Instead, write just the changed values (enables and rounding mode),
> which can be a bit more efficient.
> 
> 2019-09-19  Paul A. Clarke  <pc@us.ibm.com>
> 
> 	* sysdeps/powerpc/fpu/fenv_private.h
> 	(libc_feholdsetround_noex_ppc_ctx): Call fesetenv_mode instead
> 	of fesetenv_register.
> ---
> v2: No change.
> 
>   sysdeps/powerpc/fpu/fenv_private.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/powerpc/fpu/fenv_private.h b/sysdeps/powerpc/fpu/fenv_private.h
> index 9496026..ade0bfa 100644
> --- a/sysdeps/powerpc/fpu/fenv_private.h
> +++ b/sysdeps/powerpc/fpu/fenv_private.h
> @@ -142,7 +142,7 @@ libc_feholdsetround_noex_ppc_ctx (struct rm_ctx *ctx, int r)
>     if (__glibc_unlikely (new.l != old.l))
>       {
>         __TEST_AND_ENTER_NON_STOP (old.l, 0ULL);
> -      fesetenv_register (new.fenv);
> +      fesetenv_mode (new.fenv);
>         ctx->updated_status = true;
>       }
>     else
>

OK, with suggested cleanup to commit message.

Reviewed-By: Paul E Murphy <murphyp@linux.ibm.com>
Paul A. Clarke Sept. 23, 2019, 5:54 p.m. UTC | #2
Thanks for the review, Paul!  Question...

On 9/23/19 10:54 AM, Paul E Murphy wrote:
> On 9/19/19 1:46 PM, Paul A. Clarke wrote:
>> libc_feholdsetround_noex_ppc_ctx currently does, basically:
> The listing reads awkwardly for me. I suggest a little cleanup. There is no need to post a new patch against my suggestion.

Here you say "listing", and there is a list (1-2-3) immediately below this text, and below you say "commit message", .  Where would you like to see improvement?  Happy to change, but it's hard for me to see what you're seeing.  (And, of course, it's all perfectly clear to me since I wrote it!  ;-)

>> 1. Read FPSCR, save to context.
>> 2. Create new FPSCR value: clear enables and set new rounding mode.
>> 3. Write new value to FPSCR.
>>
>> Since other bits just pass through, there is no need to write them.
>>
>> Instead, write just the changed values (enables and rounding mode),
>> which can be a bit more efficient.
>>
>> 2019-09-19  Paul A. Clarke  <pc@us.ibm.com>
>>
>>     * sysdeps/powerpc/fpu/fenv_private.h
>>     (libc_feholdsetround_noex_ppc_ctx): Call fesetenv_mode instead
>>     of fesetenv_register.
>> ---
>> v2: No change.
>>
>>   sysdeps/powerpc/fpu/fenv_private.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sysdeps/powerpc/fpu/fenv_private.h b/sysdeps/powerpc/fpu/fenv_private.h
>> index 9496026..ade0bfa 100644
>> --- a/sysdeps/powerpc/fpu/fenv_private.h
>> +++ b/sysdeps/powerpc/fpu/fenv_private.h
>> @@ -142,7 +142,7 @@ libc_feholdsetround_noex_ppc_ctx (struct rm_ctx *ctx, int r)
>>     if (__glibc_unlikely (new.l != old.l))
>>       {
>>         __TEST_AND_ENTER_NON_STOP (old.l, 0ULL);
>> -      fesetenv_register (new.fenv);
>> +      fesetenv_mode (new.fenv);
>>         ctx->updated_status = true;
>>       }
>>     else
>>
> 
> OK, with suggested cleanup to commit message.
> 
> Reviewed-By: Paul E Murphy <murphyp@linux.ibm.com>

PC
Paul E Murphy Sept. 27, 2019, 2:27 p.m. UTC | #3
On 9/23/19 12:54 PM, Paul Clarke wrote:
> Thanks for the review, Paul!  Question...
> 
> On 9/23/19 10:54 AM, Paul E Murphy wrote:
>> On 9/19/19 1:46 PM, Paul A. Clarke wrote:
>>> libc_feholdsetround_noex_ppc_ctx currently does, basically:
>> The listing reads awkwardly for me. I suggest a little cleanup. There is no need to post a new patch against my suggestion.
> 
> Here you say "listing", and there is a list (1-2-3) immediately below this text, and below you say "commit message", .  Where would you like to see improvement?  Happy to change, but it's hard for me to see what you're seeing.  (And, of course, it's all perfectly clear to me since I wrote it!  ;-)
> 
>>> 1. Read FPSCR, save to context.
>>> 2. Create new FPSCR value: clear enables and set new rounding mode.
>>> 3. Write new value to FPSCR.
>>>
I think the change of tense and verb leading into the list threw me off. 
Feel free to commit with or without changes as the message does convey 
the intent of the patch.
diff mbox series

Patch

diff --git a/sysdeps/powerpc/fpu/fenv_private.h b/sysdeps/powerpc/fpu/fenv_private.h
index 9496026..ade0bfa 100644
--- a/sysdeps/powerpc/fpu/fenv_private.h
+++ b/sysdeps/powerpc/fpu/fenv_private.h
@@ -142,7 +142,7 @@  libc_feholdsetround_noex_ppc_ctx (struct rm_ctx *ctx, int r)
   if (__glibc_unlikely (new.l != old.l))
     {
       __TEST_AND_ENTER_NON_STOP (old.l, 0ULL);
-      fesetenv_register (new.fenv);
+      fesetenv_mode (new.fenv);
       ctx->updated_status = true;
     }
   else