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

CI (MinGW): Remove work-around needed for old versions of LLVM Flang #4180

Merged
merged 1 commit into from
Nov 26, 2024

Conversation

mmuetzel
Copy link
Contributor

@mmuetzel mmuetzel commented Aug 5, 2023

Some features either don't compile or don't work correctly with LLVM Flang before version 17.
This PR tries to help users to avoid these known-bad configurations by emitting a message and adapting the corresponding flags.

It's still unclear if both of these known-bad configurations will actually be fixed in LLVM Flang 17 or if one of them is actually an issue that is platform-dependent (i.e., only Windows or MinGW). In the latter case, the conditions could still be adapted in the future.

The flags need to be adapted early on. At least, before system_check.cmake is included via system.cmake. That required to move the check for the Fortran compiler to earlier in the configuration process.

@mmuetzel mmuetzel changed the title CMake: Check for known bad configurations CMake: Check for known bad configurations with LLVM Flang Aug 8, 2023
@mmuetzel
Copy link
Contributor Author

Updated the PR and rebased on a current head.
Building with AVX512 still leads to failing tests with LLVM Flang 17 on Windows.

@MehdiChinoune
Copy link
Contributor

Any update with flang-18.1?

@MehdiChinoune
Copy link
Contributor

All tests pass with flang-18

@martin-frbg
Copy link
Collaborator

avx512 too ?

@MehdiChinoune
Copy link
Contributor

MehdiChinoune commented Apr 6, 2024

avx512 too ?

I don't know, I haven't passed NO_AVX512 to cmake and It builds (after applying #4616) and all tests pass.
I don't have an AVX512 PC to test on.

@martin-frbg
Copy link
Collaborator

I see - actually passing the tests on an avx512 cpu used to be the problem with recent llvm, but I will probably not get to recheck this before monday

@mmuetzel
Copy link
Contributor Author

Try again after MSYS2 updated to LLVM 19.

If I understand correctly, there was a lot of activity around LLVM Flang in OpenBLAS recently. I don't know which were the findings around compatible versions on which platform.

Please, let me know if I got some of the conditions wrong.

@martin-frbg
Copy link
Collaborator

My current understanding is that LLVM/flang works on non-Windows platforms since version 17, and on Windows since 19 (bar some small issues that may be misunderstandings on my part, which I hope to resolve this week).

@mmuetzel
Copy link
Contributor Author

Do you recall what the situation was regarding OpenMP? I have a faint memory that something didn't yet work correctly with LLVM Flang 17 and LLVM libomp. (But it's very much possible, I'm remembering incorrectly.)

@martin-frbg
Copy link
Collaborator

ISTR flang17 did not yet have the necessary omp_lib module (at least early on) and possibly implementation issues on the C side too - in any case it does not seem far-fetched to me to require a fairly recent version of a compiler that is officially still not fully stable.

@mmuetzel
Copy link
Contributor Author

in any case it does not seem far-fetched to me to require a fairly recent version of a compiler that is officially still not fully stable.

Sounds reasonable to me. 👍

With the proposed changes, CMake would disable AVX512 instructions with LLVM Flang before version 17 (on anything but Windows) or before version 19 (on Windows). OpenMP would be disabled for LLVM Flang before version 19 (on any operating system).

If I understand correctly, that more or less reflects what is actually supported when using that compiler.

Is the PR acceptable in this form?

@h-vetinari
Copy link
Contributor

I think the easiest here might be to just error on flang <19 (or require an explicit opt-in with a scary CMake variable). Otherwise this seems like a good idea to me.

I think there's also still some CMake issues with CMake's openmp+fortran detection on flang. For our current openblas builds, we have to use

:: https://discourse.cmake.org/t/how-to-find-openmp-with-clang-on-macos/8860
set "CMAKE_EXTRA=-DOpenMP_ROOT=%LIBRARY_LIB%"
:: not picked up by `find_package(OpenMP)` for some reason
set "CMAKE_EXTRA=-DOpenMP_Fortran_FLAGS=-fopenmp -DOpenMP_Fortran_LIB_NAMES=libomp"

@martin-frbg
Copy link
Collaborator

I'm actually unsure right now if flang+openmp wasn't purely a Windows problem with flang18 , so excluding it for Linux and others would be a step backwards.
And given that the llvm team are planning to provide flang in their "official" Windows binary package only after the rename back from flang-new that seems to be scheduled for llvm20, maybe the Windows options including current CMake specials should simply be added to the documentation ?

@mmuetzel
Copy link
Contributor Author

OK.
So, should all changes to the build rules be removed from this PR? And given that MSYS2 updated to LLVM 19, should only the CI rules be adapted?

@martin-frbg
Copy link
Collaborator

At least I think so - there should be no (more) need to fight outdated versions on Windows where they are&were not readily available, and similarly on Linux where llvm/flang matured a good bit earlier

@mmuetzel mmuetzel changed the title CMake: Check for known bad configurations with LLVM Flang CI (MinGW): Remove work-around needed for old versions of LLVM Flang Nov 26, 2024
@mmuetzel
Copy link
Contributor Author

Re-titled and dropped all changes but for the CI rules on MinGW.

@martin-frbg
Copy link
Collaborator

Thank you

@martin-frbg martin-frbg merged commit 8e8003a into OpenMathLib:develop Nov 26, 2024
81 of 84 checks passed
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.

4 participants