-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[Flang] Move builtin .mod generation into runtimes (Reapply #137828) #169638
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
Conversation
|
@jhuber6 Based on the feedback in #137828, I made it easier to build the Fortran modules for nvptx and amdgpu targets. It is now possible to set both FLANG_RT_ENABLE_STATIC and FLANG_RT_ENABLE_SHARED to off, meaning no library is built, only the modules files. This is the default if the target is nvptx or amdgpu. However, this overrides the (experimental) default behavior of #131826. I could not get the gpu_sources to compile since it cannot find include files such as In the spirit off adding |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
|
Thank you, Michael! The patchs seems to be working on my side (after some adjustments downstream). So it is good to go for the next week merge (once you merge it, I will resolve conflicts on my side). I do not know about the experimental direct GPU build. @jhuber6 can you please comment on Michael's question above? |
|
After working around the CMAKE_Fortran_PREPROCESS_SOURCE failure, openmp-offload-amdgpu-clang-flang is failing with a different problem: The builder is running Fortran+OpenMP offload tests on amdgpu-amd-amdhsa. For these to compile, omp_lib.mod and the builtin modules must be compiled for amdgpu-amd-amdhsa as well.
|
|
One problem with enabling flang-rt on the GPU is that it has a dependency on libc / libc++ on the GPU as well. Maybe there's a way to make it work there just to build the module files? |
This is what FLANG_RT_ENABLE_STATIC=OFF FLANG_RT_ENABLE_SHARED=OFF does. It should only build the .f90 files which have no dependency to libc or libc++ headers, and only builds object libraries, so there is no linking step which would require libc. My question specifically is whether it is OK to make this the default on nvptx/amdgpu targets which this PR currently does. |
|
Thank you for the changes and for postponing the merge, @Meinersbur! It is good to go! |
|
On AIX, there is only On Linux, I can see all the modules generated. |
|
@kkwli Without access to an AIX system, how can I reproduce the issue? I do see all the .f90 files built here:
This is the wrong path for the modules files. If using a bootstrapping build, it should be in |
@Meinersbur My bad! I messed up the build environment. Things look good now. All .mod files are generated on AIX. Sorry for the false alarm. |
DanielCChen
left a comment
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.
LGTM.
Thanks!
kkwli
left a comment
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.
LG. Thanks.
Extracted out of #169638. The motivation is that we want to build Fortran module files for device triples (amdgpu-amd-amdhsa and nvptx64-nvidia-cuda) as well, but the runtimes/ directory is only included for host devices. This PR itself should not change anything, including that omp_lib.mod is only built on host devices triple. Some dependencies used for building omp_lib.mod are hoisted out of runtimes/ as well. IMHO they all make sense to hoist, e.g. LIBOMP_VERSION_MAJOR/LIBOMP_VERSION_MINOR should be usable in the entire OpenMP subproject.
…einersbur/flang_builtin-mods_2
…inersbur/flang_builtin-mods_2
…einersbur/flang_builtin-mods_2
…inersbur/flang_builtin-mods_2
Extracted out of llvm#169638. The motivation is that we want to build Fortran module files for device triples (amdgpu-amd-amdhsa and nvptx64-nvidia-cuda) as well, but the runtimes/ directory is only included for host devices. This PR itself should not change anything, including that omp_lib.mod is only built on host devices triple. Some dependencies used for building omp_lib.mod are hoisted out of runtimes/ as well. IMHO they all make sense to hoist, e.g. LIBOMP_VERSION_MAJOR/LIBOMP_VERSION_MINOR should be usable in the entire OpenMP subproject.
…lvm#137828) (llvm#169638)" needs more work This reverts commit 7675fc7.
|
This broke the runtimes build on Windows when targeting a non-Windows target with the following error: Would it be possible to revert the change while this is being investigated? Since this makes significant changes to the runtimes build that affects non-Flang users, I'd also appreciate to be included as a reviewer on the reland. |
…137828) (#169638)" This reverts commit 7675fc7. Requested in PR: #169638 (comment)
|
This indicates that You were included as reviewer in the original PR #137828 without participarting in it. LLVM's workflow explicitly mentions post-commit reviews. Were have been updating our CI builders and are already building patches on top of it, so a revert would come with some friction to us. |
|
Same on my side. A revert will be painful. |
Too late unfortunately. I talked with our maintainers and we will keep this PR applied downstream for now. |
… (Reapply #137828) (#169638)" This reverts commit 7675fc7. Requested in PR: llvm/llvm-project#169638 (comment)
Reapplication of #137828, changes:
try_compiledoes not forward manually-defined compiler flang variables to the test build environment; instead of just a negative test result, it aborts the configuration step itself. To be fair, manually defining these variables is deprecated since at least CMake 3.6.-target=,-O2,-O3Move building the .mod files from openmp/flang to openmp/flang-rt using a shared mechanism. Motivations to do so are:
iso_c_binding.modwhich encodes the target's ABI. Constants such asc_long_doublealso have different values. Most other modules have#ifdef-enclosed code as well. For instance this caused offload targets nvptx64-nvidia-cuda/amdgpu-amd-amdhsa to use the modules files compiled for the host which may contrain uses of the types REAL(10) or REAL(16) not available for nvptx/amdgpu.#146876
#128015
#129742
#158790
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 in the CMakeLists.txt.
It allows using Fortran itself to implement Flang-RT. Currently, only
iso_fortran_env_impl.f90emits 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 (PR [Flang-RT] Remerge iso_fortran_env and iso_fortran_env_impl #169525).If using Flang for cross-compilation or target-offloading, flang-rt must now be compiled for each target not only for the library, but also to get the target-specific module files. For instance in a bootstrapping runtime build, this can be done by adding:
-DLLVM_RUNTIME_TARGETS=default;nvptx64-nvidia-cuda;amdgpu-amd-amdhsa.Some new dependencies come into play:
lib_omp.modandlib_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 intrinsic modules could be marked withREQUIRES: flangrt-modules.lib_omp.modandlib_omp_kinds.modthose are already marked withopenmp_runtime.As intrinsic are now specific to the target, their location is moved from
include/flangto<resource-dir>/finclude/flang/<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.cmakeandruntimes/CMakeLists.txtso they can be used by both, openmp and flang-rt. Potentially the mechnism could also be shared by other libraries such as compiler-rt.fincludewas chosen becausegfortranuses it as well and avoids misuse such as#include <flang/iso_c_binding.mod>. The search location is now determined byToolChainin the driver, instead of by the frontend. Another subdirectoryflangavoid accidental inclusion of gfortran-modules which due to compression would result in user-unfriendly errors. Now the driver adds-fintrinsic-module-pathfor that location to the frontend call (Just like gfortran does).-fintrinsic-module-pathhad to be fixed for this because ironically it was only added tosearchDirectories, but notintrinsicModuleDirectories_. Since the driver determines the location, tests invokingflang -fc1andbbcmust 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=....