diff mbox series

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

Message ID CAFULd4Ybh-wYYDLFUccrZewzq1G+Jwy0Ot7YP8_anFRK5rMoBg@mail.gmail.com
State New
Headers show
Series [fortran,ieee] : PR 88678, Many gfortran.dg/ieee/ieee_X.f90 test cases fail starting with r267465 | expand

Commit Message

Uros Bizjak Jan. 30, 2019, 7:11 p.m. UTC 
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.

Comments

Janne Blomqvist Jan. 30, 2019, 8:51 p.m. UTC | #1 
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.
Uros Bizjak Jan. 31, 2019, 7:46 a.m. UTC | #2 
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.
Steve Ellcey Feb. 7, 2019, 11:53 p.m. UTC | #3 
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
Uros Bizjak Feb. 8, 2019, 9:42 a.m. UTC | #4 
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.
Steve Ellcey Feb. 8, 2019, 6:49 p.m. UTC | #5 
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
diff mbox series

Patch

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);
 }