-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[Flang][OpenMP] Move builtin .mod generation into runtimes #137828
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the main limitation here? If this is just a file dependency it should be identical to how all the OpenMP tests depend on omp.h
being in the resource directory. IMHO this is trivial if we do a runtimes build, since we can just require that openmp;flang-rt
are in the same toolchain, which then gives us well defined access to openmp
's CMake targets so long as it's listed before flang-rt
.
While I appreciate the review, it is not yet in the state that warants one. It is still in an experimentation stage, so I did not yet care about formatting. There are also a lot of changes in here that will eventually not be needed. Goals are:
Sounds relatively simple, but there have been many small issues, starting with CMake's misspelling of CMAKE_Fortran_BUILDING_INSTRINSIC_MODULES. |
Just want to make sure: Should it be |
That is correct, I forgot the version number that is part of the resource directory. |
907d3d5
to
839198d
Compare
✅ With the latest revision this PR passed the Python code formatter. |
With toolchain you mean a bootstrapping build with
|
@@ -6,8 +6,8 @@ | |||
!----------------------------------------- | |||
! FRONTEND FLANG DRIVER (flang -fc1) | |||
!----------------------------------------- | |||
! RUN: %flang_fc1 -fsyntax-only %s 2>&1 | FileCheck %s --allow-empty --check-prefix=WITHOUT | |||
! RUN: not %flang_fc1 -fsyntax-only -fintrinsic-modules-path %S/Inputs/ %s 2>&1 | FileCheck %s --check-prefix=GIVEN | |||
! RUN: %flang_bare -fsyntax-only %s %intrinsic_module_flags 2>&1 | FileCheck %s --check-prefix=WITHOUT --allow-empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this RUN command supposed to be invoked without the intrinsic module flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this PR, the default intrinsic path is now determined by the driver, not by the frontend. To emulate driver behavior, the test needs to add the default intrinsics path manually.
Alternatively, this test could invoke the driver instead. It's in the test/Driver
directory after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional notes:
%flang_bare
actually is the driver (i.e. not flang -fc1
, have to change the comments). The problem is that the intrinsic modules may not be in the resource directory, but in the directory defined with cmake -DFLANG_INTRINSIC_MODULES_DIR=<path>
, as otherwise it would not be possible run check-flang tests in a Flang-standalone build (that does not build flang-rt, hence no intrinsic modules in the default location).
FLANG_INTRINSIC_MODULES_DIR
is implemented by substituting flang
with flang -fintrinsic-modules-path=${FLANG_INTRINSIC_MODULES_DIR}
, i.e. always comes first in the command line. This means it will always have priority over -fintrinsic-modules-path=%S/Inputs/
. This is incompatible with the second RUN line that depends on the files in %S/Inputs/
being picked up.
Hence, this test adds -fintrinsic-modules-path=${FLANG_INTRINSIC_MODULES_DIR}
(if necessary) after -fintrinsic-modules-path=%S/Inputs/
, since with a %flang
substitution it is not possible to put it behind all other flags. %flang_bare
avoids that -fintrinsic-modules-path=${FLANG_INTRINSIC_MODULES_DIR}
is (also) added before %S/Inputs/
.
One could redesign this test, i.e. that it expects the default intrinsic modules to be matched instead of the custom ones, not relying on priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how flang tests that depend on fortran_type_info.mod are going to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klausler
If building using a bootstrapping configuration (-DLLVM_ENABLE_RUNTIMES=flang-rt
, implicitly added when -DLLVM_ENABLE_PROJECTS=flang
), the mod files are automatically put into the Clang/Flang resource directory as a dependency of check-flang
. If explictly disabled using -DFLANG_ENABLE_FLANG_RT=OFF
or in a Flang-standalone build (that currently does not support LLVM_ENABLE_RUNTIMES), Flang-RT can be compiled separately and FLANG_INTRINSIC_MODULES_DIR
pointed to it. lit.cfg.py
will add the path via -fintrinsic-module-path
to each flang
/%flang_fc1
call. If FLANG_INTRINSIC_MODULES_DIR is not set then tests in test/
will be skipped.
✅ With the latest revision this PR passed the C/C++ code formatter. |
The default build of openmp (`cmake -S <llvm-project>/runtimes -DLLVM_ENABLE_RUNTIMES=openmp`) current fails with ``` CMake Error at /home/meinersbur/src/llvm/flangrt/_src/cmake/Modules/GetClangResourceDir.cmake:17 (string): string sub-command REGEX, mode MATCH needs at least 5 arguments total to command. Call Stack (most recent call first): /home/meinersbur/src/llvm/flangrt/_src/openmp/CMakeLists.txt:126 (get_clang_resource_dir) ``` The reason is that because it is not a bootstrapping-build, the clang resource dir that it intends to write files such as `omp-tools.h` into, is unavailable. Using the Clang resource dir for writing files is conceptually broken, as that dir might be located in `/usr/lib/clang/<version>/`. Writing to it is only intended in bootstrapping builds where Clang is built alongside openmp. This patch unifies the identification of being in a bootstrapping built. The same `LLVM_TREE_AVAILABLE` definition is going to be used in #137828. No reason for each runtime to define its own.
The default build of openmp (`cmake -S <llvm-project>/runtimes -DLLVM_ENABLE_RUNTIMES=openmp`) current fails with ``` CMake Error at /home/meinersbur/src/llvm/flangrt/_src/cmake/Modules/GetClangResourceDir.cmake:17 (string): string sub-command REGEX, mode MATCH needs at least 5 arguments total to command. Call Stack (most recent call first): /home/meinersbur/src/llvm/flangrt/_src/openmp/CMakeLists.txt:126 (get_clang_resource_dir) ``` The reason is that because it is not a bootstrapping-build, the clang resource dir that it intends to write files such as `omp-tools.h` into, is unavailable. Using the Clang resource dir for writing files is conceptually broken, as that dir might be located in `/usr/lib/clang/<version>/`. Writing to it is only intended in bootstrapping builds where Clang is built alongside openmp. This patch unifies the identification of being in a bootstrapping built. The same `LLVM_TREE_AVAILABLE` definition is going to be used in llvm#137828. No reason for each runtime to define its own.
ping |
Should we choose a different module path than |
Move building the .mod files from openmp/flang to openmp/flang-rt using a shared mechanism. Motivations to do so are:
Most modules are target-dependent and need to be re-compiled for each target separate, which is something the LLVM_ENABLE_RUNTIMES system already does. Prime example is
iso_c_binding.mod
which encodes the target's ABI. Most other modules have#ifdef
-enclosed code as well.CMake has support for Fortran that we should use. Among other things, it automatically determines module dependencies so there is no need to hardcode them manually.
It allows using Fortran itself to implement Flang-RT. Currently, only
iso_fortran_env_impl.f90
emits object files that are needed by Fortran applications ([flang] Linker for non-constant accesses to kind arrays (integer_kind, logical_kind, real_kind) #89403). The workaround of [flang][runtime] Build ISO_FORTRAN_ENV to export kind arrays as linkable symbols #95388 could be reverted.Some new dependencies come into play:
lib_omp.mod
andlib_omp_kinds.mod
. Currently, if flang-rt is not found then the modules are not built-DFLANG_INTRINSIC_MODULES_DIR=<path>
, e.g. in a flang-standalone build. Alternatively, the test needing any of the instrinsic modules could be marked withREQUIRES: flangrt-modules
which would affect ~217 files.lib_omp.mod
andlib_omp_kinds.mod
those are already marked withopenmp_runtime
.As intrinsic are now specific to the target, their location os moved from
include/flang
to<resource-dir>/finclude/<triple>
. The mechnism to compute the location have been moved from flang-rt (previously used to compute the location oflibflang_rt.*.a
) to common locations incmake/GetToolchainDirs.cmake
andruntimes/CMakeLists.txt
so they can be used by both, openmp and flang-rt. Potentially the mechnism could also be shared by other libraries such as compiler-rt.finclude
was chosen becausegfortran
uses it as well and avoids misuse such as#include <flang/iso_c_binding.mod>
. The search location is now determined byToolChain
in the driver, instead of by the frontend. The now the driver adds-fintrinsic-module-path
for that location to the frontend call (Just like gcc does).-fintrinsic-module-path
had to be fixed for this because ironically it was only added tosearchDirectories
, but notintrinsicModuleDirectories_
. Since the driver determines the location, tests invokingflang -fc1
andbbc
must also be passed the location by llvm-lit. This works like llvm-lit does for finding the include dirs for Clang using-print-file-name=...
.Related PRs:
SHELL:
workaround)