diff mbox series

[v2,6/7] alpha: Fix fesetexceptflag (BZ 30998)

Message ID 20231106132713.953501-7-adhemerval.zanella@linaro.org
State Committed
Commit 80a40a9e14d9a01e3f70c5b37ecd1da83033b6de
Headers
Series Multiple floating-point environment fixes |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Adhemerval Zanella Nov. 6, 2023, 1:27 p.m. UTC 
  From: Bruno Haible <bruno@clisp.org>

It clears some exception flags that are outside the EXCEPTS argument.

It fixes math/test-fexcept on qemu-user.
---
 sysdeps/alpha/fpu/fsetexcptflg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Carlos O'Donell Nov. 6, 2023, 4:54 p.m. UTC | #1 
On 11/6/23 08:27, Adhemerval Zanella wrote:
> From: Bruno Haible <bruno@clisp.org>
> 
> It clears some exception flags that are outside the EXCEPTS argument.
> 
> It fixes math/test-fexcept on qemu-user.

> ---
>  sysdeps/alpha/fpu/fsetexcptflg.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sysdeps/alpha/fpu/fsetexcptflg.c b/sysdeps/alpha/fpu/fsetexcptflg.c
> index 70f3666a6e..63eb06845d 100644
> --- a/sysdeps/alpha/fpu/fsetexcptflg.c
> +++ b/sysdeps/alpha/fpu/fsetexcptflg.c
> @@ -27,7 +27,7 @@ __fesetexceptflag (const fexcept_t *flagp, int excepts)
>    tmp = __ieee_get_fp_control ();
>  
>    /* Set all the bits that were called for.  */
> -  tmp = (tmp & ~SWCR_STATUS_MASK) | (*flagp & excepts & SWCR_STATUS_MASK);
> +  tmp ^= (tmp ^ *flagp) & excepts & SWCR_STATUS_MASK;

Does this actually work?

Assume excepts is FE_INVALID, and *flagp bit 17 is 0.
Assume currently bit 17 is 1.

tmp ^ *flagp       => bit 17 is still 1, even though bit 17 in flagp is 0.
& excepts          => bit 17 is still 1.
& SWCR_STATUS_MASK => bit 17 is still 1.
^=                 => bit 17 is still 1.

The operation will set a bit, but won't clear it?

I would expect (taken from hppa code I wrote for that port):

/* Clear all status bits we care about.  */
tmp = tmp & ~(excepts & SWCR_STATUS_MASK);
/* Install the new ones.  */
tmp |= *flagp & excepts & SWCR_STATUS_MASK;

>  
>    /* And store it back.  */
>    __ieee_set_fp_control (tmp);
Bruno Haible Nov. 6, 2023, 5:36 p.m. UTC | #2 
Carlos O'Donell wrote:
> I would expect (taken from hppa code I wrote for that port):
> 
> /* Clear all status bits we care about.  */
> tmp = tmp & ~(excepts & SWCR_STATUS_MASK);
> /* Install the new ones.  */
> tmp |= *flagp & excepts & SWCR_STATUS_MASK;

This code is correct. And, as I wrote in
https://sourceware.org/bugzilla/show_bug.cgi?id=30998#c2
the code with the double-xor is faster, because it uses one less
instruction.

Basically the code you proposed masks the bits to clear
and the bits to set separately. Whereas the code with the double-xor
masks the bits to *change* - a single AND instead of two.

> >    /* Set all the bits that were called for.  */
> > -  tmp = (tmp & ~SWCR_STATUS_MASK) | (*flagp & excepts & SWCR_STATUS_MASK);
> > +  tmp ^= (tmp ^ *flagp) & excepts & SWCR_STATUS_MASK;
> 
> Does this actually work?

Yes: It computes the bits to *change*, then applies the mask, then applies
the changes.

For bits in the mask, it does
        tmp.bit[i] ^= tmp.bit[i] ^ (*flagp).bit[i];
which simplifies to
        tmp.bit[i] = (*flagp).bit[i];

For bits outside the mask, it does
        tmp.bit[i] ^= 0;
i.e. it leaves the bit unchanged.

I'm using this same idiom also in gnulib
https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/fenv-except-state-set.c;h=2035f61e2714126e432c0c1f29450f18f58d81f1;hb=HEAD#l182
and I have verified that it works.

> Assume excepts is FE_INVALID, and *flagp bit 17 is 0.
> Assume currently bit 17 is 1.
> 
> tmp ^ *flagp       => bit 17 is still 1, even though bit 17 in flagp is 0.
> & excepts          => bit 17 is still 1.
> & SWCR_STATUS_MASK => bit 17 is still 1.
> ^=                 => bit 17 is still 1.

In the last step: 1 xor 1 is 0.

Bruno
Carlos O'Donell Nov. 6, 2023, 6:15 p.m. UTC | #3 
On 11/6/23 12:36, Bruno Haible wrote:
> I'm using this same idiom also in gnulib
> https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=blob;f=lib/fenv-except-state-set.c;h=2035f61e2714126e432c0c1f29450f18f58d81f1;hb=HEAD#l182
> and I have verified that it works.

That works for me. Thank you for verifying that.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>
diff mbox series

Patch

diff --git a/sysdeps/alpha/fpu/fsetexcptflg.c b/sysdeps/alpha/fpu/fsetexcptflg.c
index 70f3666a6e..63eb06845d 100644
--- a/sysdeps/alpha/fpu/fsetexcptflg.c
+++ b/sysdeps/alpha/fpu/fsetexcptflg.c
@@ -27,7 +27,7 @@  __fesetexceptflag (const fexcept_t *flagp, int excepts)
   tmp = __ieee_get_fp_control ();
 
   /* Set all the bits that were called for.  */
-  tmp = (tmp & ~SWCR_STATUS_MASK) | (*flagp & excepts & SWCR_STATUS_MASK);
+  tmp ^= (tmp ^ *flagp) & excepts & SWCR_STATUS_MASK;
 
   /* And store it back.  */
   __ieee_set_fp_control (tmp);