[PATCH, fortran ieee]: Clear stalled interrupt flags in glibc set_fpu_trap_exceptions

[PATCH, fortran ieee]: Clear stalled interrupt flags in glibc set_fpu_trap_exceptions

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

Hello!

When changing trap masks, it is necessary to clear pending traps to
prevent firing spurious interrupts.  Attached patch also optimizes
set_fpu_trap_exceptions function considerably to only call
feenableexcept and fedisableexcept functions each once.

2019-01-29  Uroš Bizjak  <ubizjak@gmail.com>

    * config/fpu-glibc.h (set_fpu_trap_exceptions): Clear stalled
    exception flags before changing trap mode.  Optimize to call
    feenableexcept and fedisableexcept only once.

Patch was bootstrapped and regression tested on alphaev68-linux-gnu,
where it fixes gfortran.dg/ieee/ieee_10.f90 failures.

OK for mainline?

Uros.

[-- Attachment #2: f.diff.txt --]
[-- Type: text/plain, Size: 1857 bytes --]

Index: config/fpu-glibc.h
===================================================================
--- config/fpu-glibc.h	(revision 268248)
+++ config/fpu-glibc.h	(working copy)
@@ -39,48 +39,56 @@
 
 void set_fpu_trap_exceptions (int trap, int notrap)
 {
+  int mode_set = 0, mode_clr = 0;
+
 #ifdef FE_INVALID
   if (trap & GFC_FPE_INVALID)
-    feenableexcept (FE_INVALID);
+    mode_set |= FE_INVALID;
   if (notrap & GFC_FPE_INVALID)
-    fedisableexcept (FE_INVALID);
+    mode_clr |= FE_INVALID;
 #endif
 
 /* Some glibc targets (like alpha) have FE_DENORMAL, but not many.  */
 #ifdef FE_DENORMAL
   if (trap & GFC_FPE_DENORMAL)
-    feenableexcept (FE_DENORMAL);
+    mode_set |= FE_DENORMAL;
   if (notrap & GFC_FPE_DENORMAL)
-    fedisableexcept (FE_DENORMAL);
+    mode_clr |= FE_DENORMAL;
 #endif
 
 #ifdef FE_DIVBYZERO
   if (trap & GFC_FPE_ZERO)
-    feenableexcept (FE_DIVBYZERO);
+    mode_set |= FE_DIVBYZERO;
   if (notrap & GFC_FPE_ZERO)
-    fedisableexcept (FE_DIVBYZERO);
+    mode_clr |= FE_DIVBYZERO;
 #endif
 
 #ifdef FE_OVERFLOW
   if (trap & GFC_FPE_OVERFLOW)
-    feenableexcept (FE_OVERFLOW);
+    mode_set |= FE_OVERFLOW;
   if (notrap & GFC_FPE_OVERFLOW)
-    fedisableexcept (FE_OVERFLOW);
+    mode_clr |= FE_OVERFLOW;
 #endif
 
 #ifdef FE_UNDERFLOW
   if (trap & GFC_FPE_UNDERFLOW)
-    feenableexcept (FE_UNDERFLOW);
+    mode_set |= FE_UNDERFLOW;
   if (notrap & GFC_FPE_UNDERFLOW)
-    fedisableexcept (FE_UNDERFLOW);
+    mode_clr |= FE_UNDERFLOW;
 #endif
 
 #ifdef FE_INEXACT
   if (trap & GFC_FPE_INEXACT)
-    feenableexcept (FE_INEXACT);
+    mode_set |= FE_INEXACT;
   if (notrap & GFC_FPE_INEXACT)
-    fedisableexcept (FE_INEXACT);
+    mode_clr |= FE_INEXACT;
 #endif
+
+  /* Clear stalled exception flags.  */
+  feclearexcept (FE_ALL_EXCEPT);
+
+  feenableexcept (mode_set);
+  fedisableexcept (mode_clr);
 }
 
 

Re: [PATCH, fortran ieee]: Clear stalled interrupt flags in glibc set_fpu_trap_exceptions

[Steve Kargl]

On Tue, Jan 29, 2019 at 08:46:40PM +0100, Uros Bizjak wrote:
> 
> When changing trap masks, it is necessary to clear pending traps to
> prevent firing spurious interrupts.  Attached patch also optimizes
> set_fpu_trap_exceptions function considerably to only call
> feenableexcept and fedisableexcept functions each once.
> 
> 2019-01-29  Uroš Bizjak  <ubizjak@gmail.com>

s/Uro/Uros  ?

>     * config/fpu-glibc.h (set_fpu_trap_exceptions): Clear stalled
>     exception flags before changing trap mode.  Optimize to call
>     feenableexcept and fedisableexcept only once.
> 
> Patch was bootstrapped and regression tested on alphaev68-linux-gnu,
> where it fixes gfortran.dg/ieee/ieee_10.f90 failures.
> 
> OK for mainline?
> 

Uros, 

Your decription suggests that this fixes PR fortran/88678.
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88678

I managed to identify that the issue was traceable to fpu-glibc.h,
but I got stuck going any further.

In any event, the patch looks good to me.

Thanks.

-- 
steve

Re: [PATCH, fortran ieee]: Clear stalled interrupt flags in glibc set_fpu_trap_exceptions

[Uros Bizjak]

On Tue, Jan 29, 2019 at 9:20 PM Steve Kargl
<sgk@troutmask.apl.washington.edu> wrote:
>
> On Tue, Jan 29, 2019 at 08:46:40PM +0100, Uros Bizjak wrote:
> >
> > When changing trap masks, it is necessary to clear pending traps to
> > prevent firing spurious interrupts.  Attached patch also optimizes
> > set_fpu_trap_exceptions function considerably to only call
> > feenableexcept and fedisableexcept functions each once.
> >
> > 2019-01-29  Uroš Bizjak  <ubizjak@gmail.com>
>
> s/Uro/Uros  ?

No, the original is correct. It is probably your mailer skipping
non-ascii characters.

> >     * config/fpu-glibc.h (set_fpu_trap_exceptions): Clear stalled
> >     exception flags before changing trap mode.  Optimize to call
> >     feenableexcept and fedisableexcept only once.
> >
> > Patch was bootstrapped and regression tested on alphaev68-linux-gnu,
> > where it fixes gfortran.dg/ieee/ieee_10.f90 failures.
> >
> > OK for mainline?
> >
>
> Uros,
>
> Your decription suggests that this fixes PR fortran/88678.
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88678

Actually, additional patch is needed to fully fix PR88678.
support_fpu_trap enables and disables exceptions and this may fire
spurious exceptions. Just assume that all supported flags can generate
exceptions, as is done in the additional patch, posted to PR88678.

Uros.

> I managed to identify that the issue was traceable to fpu-glibc.h,
> but I got stuck going any further.
>
> In any event, the patch looks good to me.
>
> Thanks.
>
> --
> steve

[PATCH, fortran ieee]: PR 88678, Many gfortran.dg/ieee/ieee_X.f90 test cases fail starting with r267465

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

On Wed, Jan 30, 2019 at 10:37 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> > Your decription suggests that this fixes PR fortran/88678.
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88678
>
> Actually, additional patch is needed to fully fix PR88678.
> support_fpu_trap enables and disables exceptions and this may fire
> spurious exceptions. Just assume that all supported flags can generate
> exceptions, as is done in the additional patch, posted to PR88678.

The remaining ieee_*.f90 tests and large_1.f90 test failures on
powerpc64 are fixed by the attached patch.

2019-01-30  Uroš Bizjak  <ubizjak@gmail.com>

    PR fortran/88678
    * config/fpu-glibc.h (support_fpu_trap): Do not try to enable
    exceptions to determine if exception is supported.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32} (with
appropriate config.host tweak to select fpu-glibc.header),
alphaev68-linux-gnu and as reported in the PR, on
powerpc64le-linux-gnu by Peter.

OK for mainline?

Uros.

[-- Attachment #2: f.diff.txt --]
[-- Type: text/plain, Size: 1062 bytes --]

diff --git a/libgfortran/config/fpu-glibc.h b/libgfortran/config/fpu-glibc.h
index c24bb6cbcd92..df2588e038d8 100644
--- a/libgfortran/config/fpu-glibc.h
+++ b/libgfortran/config/fpu-glibc.h
@@ -121,41 +129,7 @@ get_fpu_trap_exceptions (void)
 int
 support_fpu_trap (int flag)
 {
-  int exceptions = 0;
-  int old;
-
-  if (!support_fpu_flag (flag))
-    return 0;
-
-#ifdef FE_INVALID
-  if (flag & GFC_FPE_INVALID) exceptions |= FE_INVALID;
-#endif
-
-#ifdef FE_DIVBYZERO
-  if (flag & GFC_FPE_ZERO) exceptions |= FE_DIVBYZERO;
-#endif
-
-#ifdef FE_OVERFLOW
-  if (flag & GFC_FPE_OVERFLOW) exceptions |= FE_OVERFLOW;
-#endif
-
-#ifdef FE_UNDERFLOW
-  if (flag & GFC_FPE_UNDERFLOW) exceptions |= FE_UNDERFLOW;
-#endif
-
-#ifdef FE_DENORMAL
-  if (flag & GFC_FPE_DENORMAL) exceptions |= FE_DENORMAL;
-#endif
-
-#ifdef FE_INEXACT
-  if (flag & GFC_FPE_INEXACT) exceptions |= FE_INEXACT;
-#endif
-
-  old = feenableexcept (exceptions);
-  if (old == -1)
-    return 0;
-  fedisableexcept (exceptions & ~old);
-  return 1;
+  return support_fpu_flag (flag);
 }
 
 

Re: [PATCH, fortran ieee]: PR 88678, Many gfortran.dg/ieee/ieee_X.f90 test cases fail starting with r267465

[Janne Blomqvist]

On Wed, Jan 30, 2019 at 9:12 PM Uros Bizjak <ubizjak@gmail.com> wrote:

> On Wed, Jan 30, 2019 at 10:37 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> > > Your decription suggests that this fixes PR fortran/88678.
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88678
> >
> > Actually, additional patch is needed to fully fix PR88678.
> > support_fpu_trap enables and disables exceptions and this may fire
> > spurious exceptions. Just assume that all supported flags can generate
> > exceptions, as is done in the additional patch, posted to PR88678.
>
> The remaining ieee_*.f90 tests and large_1.f90 test failures on
> powerpc64 are fixed by the attached patch.
>
> 2019-01-30  Uroš Bizjak  <ubizjak@gmail.com>
>
>     PR fortran/88678
>     * config/fpu-glibc.h (support_fpu_trap): Do not try to enable
>     exceptions to determine if exception is supported.
>
> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32} (with
> appropriate config.host tweak to select fpu-glibc.header),
> alphaev68-linux-gnu and as reported in the PR, on
> powerpc64le-linux-gnu by Peter.
>
> OK for mainline?
>

This seems to change the only user of support_fpu_trap() that is different
from support_fpu_flag(), so with this change one could remove
support_fpu_trap() entirely and modify all callers (since it's an internal
function it's not used outside libgfortran) to call support_fpu_flag()
directly. Otherwise Ok.

-- 
Janne Blomqvist

Re: [PATCH, fortran ieee]: PR 88678, Many gfortran.dg/ieee/ieee_X.f90 test cases fail starting with r267465

[Uros Bizjak]

On Wed, Jan 30, 2019 at 9:51 PM Janne Blomqvist
<blomqvist.janne@gmail.com> wrote:
>
> On Wed, Jan 30, 2019 at 9:12 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>>
>> On Wed, Jan 30, 2019 at 10:37 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>>
>> > > Your decription suggests that this fixes PR fortran/88678.
>> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88678
>> >
>> > Actually, additional patch is needed to fully fix PR88678.
>> > support_fpu_trap enables and disables exceptions and this may fire
>> > spurious exceptions. Just assume that all supported flags can generate
>> > exceptions, as is done in the additional patch, posted to PR88678.
>>
>> The remaining ieee_*.f90 tests and large_1.f90 test failures on
>> powerpc64 are fixed by the attached patch.
>>
>> 2019-01-30  Uroš Bizjak  <ubizjak@gmail.com>
>>
>>     PR fortran/88678
>>     * config/fpu-glibc.h (support_fpu_trap): Do not try to enable
>>     exceptions to determine if exception is supported.
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32} (with
>> appropriate config.host tweak to select fpu-glibc.header),
>> alphaev68-linux-gnu and as reported in the PR, on
>> powerpc64le-linux-gnu by Peter.
>>
>> OK for mainline?
>
>
> This seems to change the only user of support_fpu_trap() that is different from support_fpu_flag(), so with this change one could remove support_fpu_trap() entirely and modify all callers (since it's an internal function it's not used outside libgfortran) to call support_fpu_flag() directly. Otherwise Ok.

Some targets only support IEEE flags (so, flags in some FP status
register), but not exceptions (traps) based on these flags that halt
the program. Currently, fpu-glibc.h assumes that all flags can
generate exceptions (that is true for the current set of gfortran
targets), but some future target wants to return 0 from
support_fpu_trap.

Uros.

Re: [PATCH, fortran ieee]: PR 88678, Many gfortran.dg/ieee/ieee_X.f90 test cases fail starting with r267465

[Steve Ellcey]

On Thu, 2019-01-31 at 08:46 +0100, Uros Bizjak wrote:
> On Wed, Jan 30, 2019 at 9:51 PM Janne Blomqvist
>
> > This seems to change the only user of support_fpu_trap() that is
> > different from support_fpu_flag(), so with this change one could
> > remove support_fpu_trap() entirely and modify all callers (since
> > it's an internal function it's not used outside libgfortran) to
> > call support_fpu_flag() directly. Otherwise Ok.
> 
> Some targets only support IEEE flags (so, flags in some FP status
> register), but not exceptions (traps) based on these flags that halt
> the program. Currently, fpu-glibc.h assumes that all flags can
> generate exceptions (that is true for the current set of gfortran
> targets), but some future target wants to return 0 from
> support_fpu_trap.
> 
> Uros.

Is this actually true for all existing gfortran targets?  Specifically
I am wondering about Arm and Aarch64.  PR 78314 says that ARM trapping
FPU exceptions are optional.

I am currently seeing gfortran.dg/ieee/ieee_6.f90 fail on my Aarch64
ThunderX box.  I wonder if we should have an Arm/Aarch64 specific
version of the fpu header file (fpu-arm.h) that would use the previous 
version of support_fpu_trap.

Steve Ellcey
sellcey@marvell.com

Re: [PATCH, fortran ieee]: PR 88678, Many gfortran.dg/ieee/ieee_X.f90 test cases fail starting with r267465

[Uros Bizjak]

On Fri, Feb 8, 2019 at 12:53 AM Steve Ellcey <sellcey@marvell.com> wrote:
>
> On Thu, 2019-01-31 at 08:46 +0100, Uros Bizjak wrote:
> > On Wed, Jan 30, 2019 at 9:51 PM Janne Blomqvist
> >
> > > This seems to change the only user of support_fpu_trap() that is
> > > different from support_fpu_flag(), so with this change one could
> > > remove support_fpu_trap() entirely and modify all callers (since
> > > it's an internal function it's not used outside libgfortran) to
> > > call support_fpu_flag() directly. Otherwise Ok.
> >
> > Some targets only support IEEE flags (so, flags in some FP status
> > register), but not exceptions (traps) based on these flags that halt
> > the program. Currently, fpu-glibc.h assumes that all flags can
> > generate exceptions (that is true for the current set of gfortran
> > targets), but some future target wants to return 0 from
> > support_fpu_trap.
> >
> > Uros.
>
> Is this actually true for all existing gfortran targets?  Specifically
> I am wondering about Arm and Aarch64.  PR 78314 says that ARM trapping
> FPU exceptions are optional.

This is assumed in the tests and in the library, please see bellow.

> I am currently seeing gfortran.dg/ieee/ieee_6.f90 fail on my Aarch64
> ThunderX box.  I wonder if we should have an Arm/Aarch64 specific
> version of the fpu header file (fpu-arm.h) that would use the previous
> version of support_fpu_trap.

The fix for PR78314 was wrong on two accounts:

a) Probing for supported traps by trying to enable and disable trap
will fire exception when the trap is disabled, but the flag in the
status register is set. You can try this on x86 by forcing it to use
unpatched previous fpu-glibc instead of fpu-387 in
libgfortran/configure.host. Please also note PR78314, comment #16,
where it is reported that the test passes if target (QEMU arm/aarch64)
supports exceptions. So, if you use the old approach in arm specific
version of the file, I suspect you will regress other tests on
arm/aarch64 targets when/if optional exceptions are supported.

b) As reported in PR78314, comment #12, libgfortran currently assumes that:

gcc/fortran/simplify.c:
gfc_expr *
simplify_ieee_support (gfc_expr *expr)
{
  /* We consider that if the IEEE modules are loaded, we have full support
     for flags, halting and rounding, which are the three functions
     (IEEE_SUPPORT_{FLAG,HALTING,ROUNDING}) allowed in constant
     expressions. One day, we will need libgfortran to detect support and
     communicate it back to us, allowing for partial support.  */

  return gfc_get_logical_expr (gfc_default_logical_kind, &expr->where,
                               true);
}

so, the reverted patch neglected this assumption. Ignoring this, we can use

--cut here--
Index: libgfortran/config/fpu-glibc.h
===================================================================
--- libgfortran/config/fpu-glibc.h      (revision 268424)
+++ libgfortran/config/fpu-glibc.h      (working copy)
@@ -129,6 +129,10 @@
 int
 support_fpu_trap (int flag)
 {
+#if defined(__arm__) || defined(__aarch64__)
+  return 0;
+#endif
+
   return support_fpu_flag (flag);
 }

--cut here--

which would in effect revert to the previous status on arm/aarch64
targets, while using correct setting for other targets, where
exception support is assumed.

Uros.

Re: [EXT] Re: [PATCH, fortran ieee]: PR 88678, Many gfortran.dg/ieee/ieee_X.f90 test cases fail starting with r267465

[Steve Ellcey]

On Fri, 2019-02-08 at 10:42 +0100, Uros Bizjak wrote:

> so, the reverted patch neglected this assumption. Ignoring this, we
> can use
> 
> --cut here--
> Index: libgfortran/config/fpu-glibc.h
> ===================================================================
> --- libgfortran/config/fpu-glibc.h      (revision 268424)
> +++ libgfortran/config/fpu-glibc.h      (working copy)
> @@ -129,6 +129,10 @@
>  int
>  support_fpu_trap (int flag)
>  {
> +#if defined(__arm__) || defined(__aarch64__)
> +  return 0;
> +#endif
> +
>    return support_fpu_flag (flag);
>  }
> 
> --cut here--
> 
> which would in effect revert to the previous status on arm/aarch64
> targets, while using correct setting for other targets, where
> exception support is assumed.
> 
> Uros.

I am not sure we want to disable this for all arm/Aarch64 chips. 
ieee_6.f90 is failing on my T88 ThunderX box with a 4.13 kernel and
passing on my T99 Thunderx2 box with a 4.15 kernel.  I don't know
if it is the hardware or the kernel that is making the difference,
I am still trying to figure that out.  I guess returning zero would be
a 'safe' choice in that it would say we do not support this on some
machines where it actually is supported but it would no longer claim
support on a machine where it is not supported.

Steve Ellcey
sellcey@marvell.com