[flang][OpenMP] Compile proper omp_lib.mod from the openmp/src/include sources
#80874
Conversation
Flang does not yet support '!$dir if' directives. So, to make this file compilable with Flang, they need to be guarded with __INTEL_COMPILER (since the if is targeting the intel compiler anyways)
|
@jpeyton52 Can you please have a look and see if the |
|
I am just wondering if we need to keep those directives. Do we support building by Intel compiler? |
I'll let someone from Intel comment on whether they still use this code downstream or what they do with it. I'd be OK with keeping them and just guarding them for the time being. |
There was a problem hiding this comment.
For Intel Fortran compiler, there would need to be an additional flag during module compilation, -fpp, to enable preprocessing. I think gfortran is OK without the flag (and they use -cpp). I don't know about other Fortran compilers.
Basically, a two-step process in the CMake where:
- cmake/config-ix.cmake: add
libomp_check_fortran_flag(-fpp LIBOMP_HAVE_FPP_FORTRAN_FLAG) cmake/LibompHandleFlags.cmake: addlibomp_append(fflags_local -fpp LIBOMP_HAVE_FPP_FORTRAN_FLAG)
The is a single Fortran flag example (-m32) to see in each file.
The compiler won't need it, if the extension changed from So, may I assume that Intel is generally fine with the change I'm proposing to make the file compilable with Flang? |
Haha! I wasn't aware of this Fortran oddity.
Yes, the change looks fine. I suppose you just need to rename the file |
|
@mjklemm Since we are removing the existing omp_lib in the flang directory, it will be good to have a quick RFC to ensure all stakeholders are fine with it. |
Good idea. Thanks! I have the CMake in place to build the right omp_lib module, but some tests are still failing. I will have to debug this and see where this is coming from. But I will create the RFC hopefully today or tomorrow. |
One immediate change to make would be to change the buildkite configuration to include "openmp" in the project list right after flang. That would slight increase the build time for each push to PR, as it would then build the OpenMP runtime, too. Not sure if that's desired. |
Hi Kiran, more of an info, I guess: We have a Flang+OpenMP buildbot set up in staging for post-commit testing (https://lab.llvm.org/staging/#/builders/140) of flang and openmp (host + gpu offload). We are working on moving that to production. I also talked with Michael offline and made him aware that our bots all build OpenMP as a runtime, which I believe he mentioned makes another change necessary. |
If the post-commit CI of @jplehr is enabled then this is not required at the moment. But I do not think the increase would matter. We might have to check more widely in a discourse post.
Thanks, @jplehr, for the info and your work here. It is great to have more testing for OpenMP+Flang upstream.
Did you mean "necessary" or "unnecessary"? |
|
I meant "necessary", but @mjklemm would need to confirm that I understood that correctly. |
Yes, that's correct. When building the OpenMP runtime via LLVM_ENABLE_RUNTIMES, it seems that the |
|
Alrighty, I have also been able to test this on Windows and everything seems to be working fine there, too. Anyone objecting to merge this? If not, please review and approve. :-) |
As discussed in Discourse and proposed in llvm/llvm-project#80874, Flang will start to use C/C++ header files and Fortran modules from OpenMP runtime. When the mentioned PR lands, all of Flang's OpenMP tests will be skipped as unsupported, unless the OpenMP project is enabled.
…tion Conflicts: flang/test/lit.cfg.py flang/test/lit.site.cfg.py.in
…lude` sources (llvm#80874) This PR changes the build system to use use the sources for the module `omp_lib` and the `omp_lib.h` include file from the `openmp` runtime project and not from a separate copy of these files. This will greatly reduce potential for inconsistencies when adding features to the OpenMP runtime implementation. When the OpenMP subproject is not configured, this PR also disables the corresponding LIT tests with a "REQUIRES" directive at the beginning of the OpenMP test files. --------- Co-authored-by: Valentin Clement (バレンタイン クレメン) <clementval@gmail.com>
As discussed in Discourse and proposed in llvm/llvm-project#80874, Flang will start to use C/C++ header files and Fortran modules from OpenMP runtime. When the mentioned PR lands, all of Flang's OpenMP tests will be skipped as unsupported, unless the OpenMP project is enabled.
Hi @mjklemm, are you planning to work on supporting OpenMP runtime when built via LLVM_ENABLE_RUNTIMES in the near future? If not, I can try to implement it. This would make things easier for the buildbots, that build OpenMP via LLVM_ENABLE_RUNTIMES by default and that also don't specify the projects directly with LLVM_ENABLE_PROJECTS. I've just noticed it now, after enabling OpenMP on some Flang bots with llvm/llvm-zorg#137, that many Flang OpenMP tests are still being skipped. |
|
It is supported. You can build the OpenMP runtime either way. |
|
The .mod file should be produced. It just ends up in a different place than when building the OpenMP runtime as a project. It should be in runtimes/runtimes-bins/openmp/src, IIRC. |
|
The .mod file was produced, but it seems it wasn't found by Flang: Also, in https://lab.llvm.org/buildbot/#/builders/173/builds/30842 it can be seen that many OpenMP tests are being skipped. They were running before this patch landed. |
|
These messages are a bit misleading. There should be another one that confirms that the .mod was built via the runtime build settings. We can check if we can enable the LIT tests again, but that means that we'd have to supply the proper build flag pointing to where the .mod file lives. A problem also is that when building a project, LLVM sets LLVM_TOOL for the project, but it does not for when building that thing as a runtime. This in turn means, there's no easy way to determine if the OpenMP runtime was built at all. That's not ideal at this point, but I will continue looking at this and trying to improve the situation and eventually get to a state where the tests are back on. |
|
Ok, thanks. Indeed there is another message mentioning the build of the .mod files.
|
|
I guess I should file a PR to improve the messages a bit. I tried to be brief. Too brief, I guess. 😀 |
This PR changes the build system to use use the sources for the module
omp_liband theomp_lib.hinclude file from theopenmpruntime project and not from a separate copy of these files. This will greatly reduce potential for inconsistencies when adding features to the OpenMP runtime implementation.When the OpenMP subproject is not configured, this PR also disables the corresponding LIT tests with a "REQUIRES" directive at the beginning of the OpenMP test files.