Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for NAG Compiler #5003

Merged
merged 6 commits into from
Dec 4, 2024
Merged

Conversation

mathomp4
Copy link
Contributor

@mathomp4 mathomp4 commented Dec 4, 2024

Closes #5002

As detailed in the above issue, this PR collects changes for the NAG compiler using CMake.

The changes are:

  1. Adding a NAG bit to cmake/fc.cmake. The flags here are mainly from both Makefile.system and from Adding NAG Fortran compiler (nagfor) support in the CMake build system. Reference-LAPACK/lapack#686 via @ACSimon33
  2. Adding the NAG PIC flag to cmake/system.cmake
  3. Converting the ctest CALL ABORT calls to Fortran Standard ERROR STOP
  4. Various Fortran Standard issues in lapack-netlib (via Adding NAG Fortran compiler (nagfor) support in the CMake build system. Reference-LAPACK/lapack#686 and Fixes for the NAG Fortran compiler Reference-LAPACK/lapack#951, again thanks to @ACSimon33)
  5. A weird CMake oddity in cmake/lapack.cmake

I'll go a bit more into that last one. Apparently, the line:

    set_source_files_properties(${LA_SOURCES} PROPERTIES COMPILE_FLAGS "${LAPACK_FFLAGS}")

was duplicating Fortran flags when make was actually run. Now, for most compilers this probably isn't a bad thing, but NAG does not allow some flags to be duplicated:

[  0%] Building Fortran object CMakeFiles/LAPACK_OVERRIDES.dir/lapack-netlib/SRC/la_constants.f90.o
Option error: -ieee= option specified twice
Option error: -thread_safe option specified twice

To be extra safe, I only turned that off when building with NAG as I don't want to change behaviors for any other compiler.

I did run ctest with NAG and got 100% (see runctest.NAG.log) as well as GCC 14 with my changes. Hopefully the CI will catch more.

If you'd like me to run any other tests with NAG (in case ctest doesn't cover everything), let me know.

Also, as mentioned in #5002 , the use of ERROR STOP might break some very old compilers. If so, I believe I could revert back to CALL ABORT and then write an ABORT external call for NAG.

@martin-frbg
Copy link
Collaborator

As far as I can tell, the redundant addition of LAPACK_FFLAGS dates back to the introduction of CMake support in version 0.2.15, a bit over nine years ago (though I cannot seem to find hpanderson's PR from back then)

@ACSimon33
Copy link

Changes looking good to me (I'm not familiar with the OpenBLAS CMake system, though). Just a quick note on the support of ABORT in nagfor:

There are some non-standard intrinsic NAG modules that contain POSIX interfaces and, therefore, also support some C system functions (abort, atexit, ...). The module in question would be f90_unix_proc.

But changing CALL ABORT to ERROR STOP makes way more sense.

@martin-frbg martin-frbg added this to the 0.3.29 milestone Dec 4, 2024
@mathomp4
Copy link
Contributor Author

mathomp4 commented Dec 4, 2024

Changes looking good to me (I'm not familiar with the OpenBLAS CMake system, though). Just a quick note on the support of ABORT in nagfor:

There are some non-standard intrinsic NAG modules that contain POSIX interfaces and, therefore, also support some C system functions (abort, atexit, ...). The module in question would be f90_unix_proc.

But changing CALL ABORT to ERROR STOP makes way more sense.

Huh. I have never heard of that f90_unix_proc. Thanks for letting me know! In most cases, I've just moved to Fortran Standard intrinsics if there was an equivalent, but I'm sure in the past I've had some where the behavior was different and I just...assumed that was the price!

@mathomp4
Copy link
Contributor Author

mathomp4 commented Dec 4, 2024

Hmm. Not sure about the failing checks. The qemu one seems to be Github hiccup, but the Windows one I'm not sure about...

@martin-frbg
Copy link
Collaborator

All good, the Windows one is also unrelated - the free Azure CI uses a variable bunch of older hardware, so long-running build jobs tend to time out when they land on a particularly slow machine

@martin-frbg martin-frbg merged commit 4ba471d into OpenMathLib:develop Dec 4, 2024
80 of 83 checks passed
@HaoZeke
Copy link
Contributor

HaoZeke commented Dec 5, 2024

Sorry I just saw the original ping, so I'm copying the crux of my comment there (#4918 (comment)), basically, I'm not sure the Fortran 2008 compliance requirement is OK for OpenBLAS, so I'd prefer documentation for f90_unix_proc instead.

@martin-frbg
Copy link
Collaborator

Well, gfortran appears to support this language feature since about version 4.6, so I would expect this to turn into a fight of ancient vendor compilers if anything. My question was more along the lines of if you know a case where it is definitely unsupported. (I assume that the "proper error return" is important enough for the Meson environment that it cannot be solved with the previous kludge of grepping the test logs for a failure phrase)

@mathomp4
Copy link
Contributor Author

mathomp4 commented Dec 6, 2024

Well, gfortran appears to support this language feature since about version 4.6, so I would expect this to turn into a fight of ancient vendor compilers if anything. My question was more along the lines of if you know a case where it is definitely unsupported. (I assume that the "proper error return" is important enough for the Meson environment that it cannot be solved with the previous kludge of grepping the test logs for a failure phrase)

If there is a compiler it isn't supported with, the best bet would probably then be to write our own "abort" routine (openblas_abort?) that calls error stop in most cases, but can call abort in the other(s)?

@martin-frbg
Copy link
Collaborator

guess so, I understand this was your plan b in the first post ? the only drawback from my viewpoint would be that it makes the code deviate more from the Reference-LAPACK original - but as the actual tests are unaffected, that would not make it harder to import any future changes

@mathomp4
Copy link
Contributor Author

mathomp4 commented Dec 6, 2024

guess so, I understand this was your plan b in the first post ? the only drawback from my viewpoint would be that it makes the code deviate more from the Reference-LAPACK original - but as the actual tests are unaffected, that would not make it harder to import any future changes

Well, I suppose we could work with Reference-LAPACK to update their tests with error stop or something else that raises a non-zero status?

@martin-frbg
Copy link
Collaborator

certainly - it is just that there might be less motivation for it, if the change is mostly useful for Meson build support that they don't have

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NAG CMake support is incomplete/broken
4 participants