Bug 78314 - [aarch64] ieee_support_halting does not report unsupported fpu traps correctly
Summary: [aarch64] ieee_support_halting does not report unsupported fpu traps correctly
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: libfortran (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: fortran-ieee
  Show dependency treegraph
 
Reported: 2016-11-11 14:42 UTC by nsz
Modified: 2022-03-30 16:17 UTC (History)
4 users (show)

See Also:
Host:
Target: aarch64-*, arm-*
Build:
Known to work:
Known to fail: 5.4.0, 6.2.0
Last reconfirmed: 2016-11-11 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description nsz 2016-11-11 14:42:03 UTC
on aarch64 trapping fpu exceptions are optional, but
ieee_support_halting(except_flag) does not report this
correctly, so fortran tests that depend on trap bit
changes would fail:

Program aborted. Backtrace:
#0  0x20000472D3
FAIL: gfortran.dg/ieee/ieee_6.f90   -Os  execution test
Comment 1 Andrew Pinski 2016-11-11 14:51:02 UTC
No wonder that one fails on thunderx.
Comment 2 Richard Earnshaw 2016-11-11 15:31:18 UTC
current code has 

int
support_fpu_trap (int flag)
{
  return support_fpu_flag (flag);
}


In other words, it assumes that if there is a flag for noting that an exception has occurred, then it is also possible to trap on that flag being changed.  That's not a valid assumption for AArch64.
Comment 3 nsz 2016-11-16 17:27:36 UTC
Author: nsz
Date: Wed Nov 16 17:27:04 2016
New Revision: 242505

URL: https://gcc.gnu.org/viewcvs?rev=242505&root=gcc&view=rev
Log:
[PR libgfortran/78314] Fix ieee_support_halting

ieee_support_halting only checked the availability of status
flags, not trapping support.  On some targets the later can
only be checked at runtime: feenableexcept reports if
enabling traps failed.

So check trapping support by enabling/disabling it.

Updated the test that enabled trapping to check if it is
supported.

gcc/testsuite/

	PR libgfortran/78314
	* gfortran.dg/ieee/ieee_6.f90: Use ieee_support_halting.

libgfortran/

	PR libgfortran/78314
	* config/fpu-glibc.h (support_fpu_trap): Use feenableexcept.


Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/ieee/ieee_6.f90
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/config/fpu-glibc.h
Comment 4 Dominique d'Humieres 2016-11-18 15:59:02 UTC
Any plan to back-port the fix? If no, could you please close the PR?
Comment 5 nsz 2016-11-18 17:25:28 UTC
i plan to backport the fix, but it seems my fix is not correct and broke the ieee_8.fp90 test.
Comment 6 nsz 2016-11-22 10:14:24 UTC
waiting with the backport for bug 78449.
Comment 7 Jakub Jelinek 2017-05-02 15:57:16 UTC
GCC 7.1 has been released.
Comment 8 Richard Biener 2018-01-25 08:27:20 UTC
GCC 7.3 is being released, adjusting target milestone.
Comment 9 Martin Liška 2018-11-19 14:40:00 UTC
nsz: Can the bug be marked as resolved?
Comment 10 nsz 2018-11-19 18:55:42 UTC
it turns out the ieee_* functions are allowed in const expressions so they need to work at compile time too (see bug 78449), which of course won't work if they need runtime detection.

so now compile-time and runtime behaviour of ieee_support_halting is different.

so far no fortran expert clarified what is the expected semantics when you can only decide this at runtime (e.g. it might be better to always return false on targets that may not implement trapping, rather than report things inconsistently compile- vs runtime).

(i think this bug can be closed, but then the other one has to be reopened).
Comment 11 Richard Earnshaw 2018-11-20 11:32:09 UTC
(In reply to nsz from comment #10)
> it turns out the ieee_* functions are allowed in const expressions so they
> need to work at compile time too (see bug 78449), which of course won't work
> if they need runtime detection.
> 
> so now compile-time and runtime behaviour of ieee_support_halting is
> different.
> 
> so far no fortran expert clarified what is the expected semantics when you
> can only decide this at runtime (e.g. it might be better to always return
> false on targets that may not implement trapping, rather than report things
> inconsistently compile- vs runtime).
> 
> (i think this bug can be closed, but then the other one has to be reopened).

Well the 'safe' answer in this case, when you can't be sure whether or not run-time traps will occur is surely to say 'no, they can't'.  You might then get redundant code in the app that can't be reached, but at least it will still execute correctly.
Comment 12 nsz 2019-01-31 16:29:15 UTC
this got reverted because of bug 88678

and because compile time and runtime support_halting are different.

the compile time value is unconditionally true, which is wrong for
aarch64 and arm:

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

i don't know how to change this to false for IEEE_SUPPORT_HALTING
on aarch64 and arm targets, but that would be a possible fix.
Comment 13 Uroš Bizjak 2019-01-31 16:52:17 UTC
(In reply to nsz from comment #12)
> i don't know how to change this to false for IEEE_SUPPORT_HALTING
> on aarch64 and arm targets, but that would be a possible fix.

--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--
Comment 14 nsz 2019-01-31 17:41:31 UTC
(In reply to Uroš Bizjak from comment #13)
> (In reply to nsz from comment #12)
> > i don't know how to change this to false for IEEE_SUPPORT_HALTING
> > on aarch64 and arm targets, but that would be a possible fix.
> 
> --cut here--
> Index: libgfortran/config/fpu-glibc.h

that only turns the runtime check into "always false"

but the compile time check is still "always true".

which is still broken.
Comment 15 nsz 2019-01-31 17:42:54 UTC
i unassigned myself as i'm not working on this right now.
Comment 16 Christophe Lyon 2019-02-01 09:59:16 UTC
(In reply to Uroš Bizjak from comment #13)
> (In reply to nsz from comment #12)
> > i don't know how to change this to false for IEEE_SUPPORT_HALTING
> > on aarch64 and arm targets, but that would be a possible fix.
> 
> --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--

I've noticed this problem on arm and aarch64 native builds too.
But my cross-compilers (using QEMU as simulator) still pass this test. Does this mean there is a bug in QEMU?
Comment 17 Uroš Bizjak 2019-02-01 10:11:43 UTC
(In reply to Christophe Lyon from comment #16)
> I've noticed this problem on arm and aarch64 native builds too.
> But my cross-compilers (using QEMU as simulator) still pass this test. Does
> this mean there is a bug in QEMU?

Looks like QEMU generates correct exceptions.
Comment 18 nsz 2019-02-01 10:17:06 UTC
(In reply to Christophe Lyon from comment #16)
> I've noticed this problem on arm and aarch64 native builds too.
> But my cross-compilers (using QEMU as simulator) still pass this test. Does
> this mean there is a bug in QEMU?

qemu-user will just translate each guest fp operations
to host fp operations, so if the host supports traps
then you will see traps working.

it's not a bug in the sense that the arm architecture
allows trap support (it's just not required), but it's
buggy that it would not report the support correctly
(e.g. enabling traps always succeed under qemu but
traps don't happen if the underlying hw has no support)
Comment 19 Christophe Lyon 2019-02-01 10:19:46 UTC
(In reply to nsz from comment #18)

> it's not a bug in the sense that the arm architecture
> allows trap support (it's just not required), but it's
> buggy that it would not report the support correctly
> (e.g. enabling traps always succeed under qemu but
> traps don't happen if the underlying hw has no support)

OK, that was my suspicion.
So, it seems hard to report a bug against qemu, then.
Comment 20 uros 2019-02-03 16:20:07 UTC
Author: uros
Date: Sun Feb  3 16:19:36 2019
New Revision: 268492

URL: https://gcc.gnu.org/viewcvs?rev=268492&root=gcc&view=rev
Log:
2019-02-03  Uroš Bizjak  <ubizjak@gmail.com>

	PR libfortran/88678
	Revert:
	2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR libfortran/78314
	* config/fpu-glibc.h (support_fpu_trap): Use feenableexcept.

2019-02-03  Uroš Bizjak  <ubizjak@gmail.com>

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


Modified:
    branches/gcc-8-branch/libgfortran/ChangeLog
    branches/gcc-8-branch/libgfortran/config/fpu-glibc.h
Comment 21 uros 2019-02-03 16:21:37 UTC
Author: uros
Date: Sun Feb  3 16:21:06 2019
New Revision: 268493

URL: https://gcc.gnu.org/viewcvs?rev=268493&root=gcc&view=rev
Log:
2019-02-03  Uroš Bizjak  <ubizjak@gmail.com>

	PR libfortran/88678
	Revert:
	2016-11-16  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	PR libfortran/78314
	* config/fpu-glibc.h (support_fpu_trap): Use feenableexcept.

2019-02-03  Uroš Bizjak  <ubizjak@gmail.com>

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


Modified:
    branches/gcc-7-branch/libgfortran/ChangeLog
    branches/gcc-7-branch/libgfortran/config/fpu-glibc.h
Comment 22 Steve Ellcey 2019-02-07 17:41:03 UTC
It looks like the recent checkins are causing gfortran.dg/ieee/ieee_6.f90
to fail on aarch64.  Reading through the comments it looks like this isn't a new problem but the failure showing up in the GCC testsuite run is new.
Comment 23 Wilco 2019-02-19 16:27:44 UTC
(In reply to Steve Ellcey from comment #22)
> It looks like the recent checkins are causing gfortran.dg/ieee/ieee_6.f90
> to fail on aarch64.  Reading through the comments it looks like this isn't a
> new problem but the failure showing up in the GCC testsuite run is new.

Well it's a regression since it used to work in GCC7/8/9. Note it's incorrect to allow folding of these calls, for example support_fpu_flag can return true or false depending on the flag (FE_DENORMAL if not supported on most targets), and yet the constant folding always returns true for all flags.
Comment 24 Steve Ellcey 2019-02-19 22:52:06 UTC
See email strings at:

https://gcc.gnu.org/ml/fortran/2019-01/msg00276.html
https://gcc.gnu.org/ml/fortran/2019-02/msg00057.html

For more discussion.
Comment 25 Wilco 2019-02-20 17:52:36 UTC
(In reply to Steve Ellcey from comment #24)
> See email strings at:
> 
> https://gcc.gnu.org/ml/fortran/2019-01/msg00276.html
> https://gcc.gnu.org/ml/fortran/2019-02/msg00057.html
> 
> For more discussion.

Sure, it looks like we simply need to reinstate Szabolc's patch with a #ifdef arm/aarch64 around it. I need to find out whether a feclearexcept (FE_ALL_EXCEPT) is necessary on Arm targets which can trap (very few exist IIRC).
Comment 26 Alex Bennée 2019-07-11 09:16:58 UTC
Just to clear up the confusion on QEMU, we don't model any architectures that support FPU exceptions. The ieee_6 test broke recently when we fixed writes to the fpscr to make the exception enabling bits RAZ/WI as they should have been all along.

Although we can use host FPU instructions (a fairly recent addition) we don't rely on their traps and I don't believe we would have actually delivered the trap. I suspect the test might be broken in that regard that it didn't check a trap had actually been delivered but I can't be sure as my fortran foo is a little lacking.

The related QEMU bug is: https://bugs.launchpad.net/qemu/+bug/1836078
Comment 27 Maxim Kuvyrkov 2019-11-15 13:52:38 UTC
Hi,

GCC 7.5 released with this bug, manifested by 
		=== gfortran Summary ===

FAIL: gfortran.dg/ieee/ieee_6.f90   -O0  execution test
FAIL: gfortran.dg/ieee/ieee_6.f90   -O1  execution test
FAIL: gfortran.dg/ieee/ieee_6.f90   -O2  execution test
FAIL: gfortran.dg/ieee/ieee_6.f90   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
FAIL: gfortran.dg/ieee/ieee_6.f90   -O3 -g  execution test
FAIL: gfortran.dg/ieee/ieee_6.f90   -Os  execution test
XPASS: gfortran.dg/ieee/ieee_8.f90   -O0  execution test
XPASS: gfortran.dg/ieee/ieee_8.f90   -O1  execution test
XPASS: gfortran.dg/ieee/ieee_8.f90   -O2  execution test
XPASS: gfortran.dg/ieee/ieee_8.f90   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
XPASS: gfortran.dg/ieee/ieee_8.f90   -O3 -g  execution test
XPASS: gfortran.dg/ieee/ieee_8.f90   -Os  execution test

Do we care enough about this to fix on GCC-7?
Comment 28 Richard Earnshaw 2019-11-15 14:00:57 UTC
The last release of gcc-7 has now been made, so it's end-of-life and no further fixes for it will be made.
Comment 29 Maxim Kuvyrkov 2019-11-19 12:47:37 UTC
(In reply to Richard Earnshaw from comment #28)
> The last release of gcc-7 has now been made, so it's end-of-life and no
> further fixes for it will be made.

Well, yes, but I'm about to build the final Linaro GCC 7.5 cross-toolchain releases, and I could fix the problem there.  E.g., by backporting a fix from GCC-8 or reverting Uros's patch.  Backporting from gcc-8 is preferred to reverting, obviously.
Comment 30 CVS Commits 2021-04-09 12:43:51 UTC
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:f44a2713da7ea8f5abde5b3a98ddf1ab97b9175a

commit r11-8087-gf44a2713da7ea8f5abde5b3a98ddf1ab97b9175a
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Fri Apr 9 13:43:15 2021 +0100

    testsuite: Skip gfortran.dg/ieee/ieee_[68].f90 for Arm targets [PR78314]
    
    For the reasons discussed in PR78314, ieee_support_halting
    doesn't work correctly for arm* and aarch64*.  I think the
    easiest thing is to skip these tests until the PR is fixed.
    
    This doesn't mean that the PR is unimportant.  It just doesn't
    seem useful to have the unpredictable failures described in the
    PR trail given that the problem is known and has been analysed.
    
    gcc/testsuite/
            PR libfortran/78314
            * gfortran.dg/ieee/ieee_6.f90: Skip for arm* and aarch64*.
            * gfortran.dg/ieee/ieee_8.f90: Likewise.
Comment 31 Richard Sandiford 2021-04-09 12:58:02 UTC
The previous patch skips the affected tests for now, on the basis
that this PR is open and tracking the problem.  The bug is very
much still there though.