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

[Flang] LLVM_ENABLE_RUNTIMES=flang-rt #110217

Open
wants to merge 125 commits into
base: users/meinersbur/flang_runtime_move-files
Choose a base branch
from

Conversation

Meinersbur
Copy link
Member

@Meinersbur Meinersbur commented Sep 27, 2024

Extract Flang's runtime library to use the LLVM_ENABLE_RUNTIME mechanism.

Motivation:

  • Consistency with LLVM's other runtime libraries (compiler-rt, libc, libcxx, openmp offload, ...)
  • Allows compiling the runtime for multiple targets at once using the LLVM_RUNTIME_TARGETS configuration options
  • Installs the runtime into the compiler's per-target resource directory so it can be automatically found even when cross-compiling

Potential future directions:

  • Uses CMake's support for compiling Fortran files, including dependency resolution of Fortran modules
  • Improve robustness of compiling libomp.mod when openmp is available
  • Remove Flang's dependency from flang-rt's RTNAME function declarations (tblgen?)
  • Reduce Flang's build-time dependency from flang-rt's REAL(16) support

See RFC discussion at https://discourse.llvm.org/t/rfc-use-llvm-enable-runtimes-for-flangs-runtime/80826

Patch series:

Patch for lab.llvm.org buildbots:

Extract Flang's runtime library to use the LLVM_ENABLE_RUNTIME
mechanism.
@Meinersbur Meinersbur changed the base branch from main to users/meinersbur/flang_runtime_move-files September 27, 2024 18:58
Move modules back to flang

Fix check-flang

Build fixes and cleanup

Test fix

Solve mscrt lib choice

clang_rt.builtins workaround via CMAake

clang_rt.builtins workaround via Clang

Revert "clang_rt.builtins workaround via Clang"

This reverts commit 2678d0c9a0ca86cfada9351d3328119388a35609.

Fix search for libclang_rt.builtins

move magic-numbers.h to common files
Copy link

github-actions bot commented Oct 1, 2024

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:
darker --check --diff -r b58bd1ffc06f04755e7611f1f160bcf4d1d21bce...4b65d4bda6c1020928bab6c001924204d5920d70 flang-rt/test/NonGtestUnit/lit.cfg.py flang-rt/test/Unit/lit.cfg.py flang-rt/test/lit.cfg.py flang/test/lit.cfg.py
View the diff from darker here.
--- flang-rt/test/lit.cfg.py	2025-01-02 15:54:24.000000 +0000
+++ flang-rt/test/lit.cfg.py	2025-01-10 01:11:38.647483 +0000
@@ -78,23 +78,21 @@
         "%clang",
         command=FindTool("clang"),
         extra_args=isysroot_flag,
         unresolved="fatal",
     ),
-    ToolSubst("%cc",
-        command=config.cc,
-        extra_args=isysroot_flag,
-        unresolved="fatal"
-    ),
+    ToolSubst("%cc", command=config.cc, extra_args=isysroot_flag, unresolved="fatal"),
 ]
 llvm_config.add_tool_substitutions(tools)
 
 # Let tests find LLVM's standard tools (FileCheck, split-file, not, ...)
 llvm_config.with_environment("PATH", config.llvm_tools_dir, append_path=True)
 
 # Include path for C headers that define Flang's Fortran ABI.
-config.substitutions.append(("%include", os.path.join(config.flang_source_dir, "include")))
+config.substitutions.append(
+    ("%include", os.path.join(config.flang_source_dir, "include"))
+)
 
 # Library path of libflang_rt.a (for lib search path when using non-Flang driver for linking)
 config.substitutions.append(("%libdir", config.flang_rt_output_resource_lib_dir))
 
 # Additional library depedendencies the that Flang driver does not add itself.

Copy link

github-actions bot commented Oct 1, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@Meinersbur
Copy link
Member Author

Meinersbur commented Oct 1, 2024

I reverted the change that moved flang/module/*.f90 to FortranRuntime/module/*.f90. A lot of Flang tests depend on the *.mod files to be available. Moving the modules to FortranRuntime would either require moving those tests to FortranRuntime as well, or mark them as requiring the module_files (REQUIRES: module_files) and build those before check-flang:

flowchart TB
 subgraph Flang
   direction TB
   flang-new
   check-flang
 end
 subgraph FortranRuntime
   direction TB
   iso_fortran_env_impl("iso_fortran_env_impl.f90")
   lib("libFortranRuntime.a")
   module_files
   check-FortranRuntime
 end

 flang-new --> iso_fortran_env_impl --> lib --> check-FortranRuntime
 flang-new --> module_files --> check-flang
 flang-new --> check-flang
 flang-new --> check-FortranRuntime
Loading

Except the PPC modules, the mod files are not target-specific and hence would not profit from compiling multiple targets from a bootstrap build anyway. However, they are products of the Flang compiler and therefore IMHO logically fits better to the runtime. Also, it would have been nice to make use of CMake's dependency resolution rather than hardcoding Into the CMakeLists.txt. I think it is best to keep that decision out of the current patch.

Meinersbur added a commit that referenced this pull request Oct 7, 2024
Due to missing lit.cfg, will only be executing again after #110217.
@Meinersbur Meinersbur marked this pull request as ready for review October 7, 2024 20:57
@Meinersbur Meinersbur requested a review from a team as a code owner October 7, 2024 20:57
@Meinersbur Meinersbur changed the title [Flang][DRAFT] LLVM_ENABLE_RUNTIMES=FortranRuntime [Flang] LLVM_ENABLE_RUNTIMES=FortranRuntime Oct 7, 2024
@Meinersbur
Copy link
Member Author

Meinersbur commented Jan 8, 2025

It is an old problem, see #87866 (comment)

Can we raise an issue for this?

Created #122152

I don't expect anything come out of it, I think moving to LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=ON by default is deliberate.

@Meinersbur
Copy link
Member Author

Meinersbur commented Jan 9, 2025

The library is present under

$PREFIX/flang-rt/lib/x86_64-unknown-linux-gnu/libflang_rt.a

(where it got installed through the changes in this PR, without any specific overrides).

If $PREFIX is CMAKE_INSTALL_PREFIX, it is the wrong location. it should be in $PREFIX/lib/clang/20/lib/x86_64-unknown-linux-gnu/libflang_rt.a. I don't see how the flang_rt dir could get in there. Can you try with the latest update?

If $PREFIX is CMake's build directory, are you using a runtimes-standalone (non-bootstrap) build and running flang from there? Then $PREFIX for flang and flang-rt are different. flang looks into its own buildir only. You will have to -L to that directory or install them into the same CMAKE_INSTALL_PREFIX.

@Meinersbur Meinersbur marked this pull request as draft January 9, 2025 02:07
@Meinersbur Meinersbur changed the base branch from users/meinersbur/flang_runtime_move-files to users/meinersbur/flang_runtime_FortranSupport January 9, 2025 02:07
@Meinersbur Meinersbur changed the base branch from users/meinersbur/flang_runtime_FortranSupport to users/meinersbur/flang_runtime_move-files January 9, 2025 02:08
@Meinersbur Meinersbur marked this pull request as ready for review January 9, 2025 02:08
Not used by compiler-rt either
@Meinersbur Meinersbur force-pushed the users/meinersbur/flang_runtime branch from e92a529 to 301af7f Compare January 9, 2025 03:22
Copy link
Contributor

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw some minor merge damage

flang/lib/Optimizer/Transforms/CMakeLists.txt Outdated Show resolved Hide resolved
flang/lib/Parser/CMakeLists.txt Outdated Show resolved Hide resolved
flang/lib/Semantics/CMakeLists.txt Outdated Show resolved Hide resolved
@h-vetinari
Copy link
Contributor

If $PREFIX is CMAKE_INSTALL_DIR, it is the wrong location. it should be in $PREFIX/lib/clang/20/lib/x86_64-unknown-linux-gnu/libflang_rt.a. I don't see how the flang_rt dir could get in there. Can you try with the latest update?

Both flang and flang-rt get installed into the same -DCMAKE_INSTALL_PREFIX=$PREFIX. I did try with the latest state of this PR, and it runs into the same error.

I then tried shifting -DLLVM_ENABLE_RUNTIMES="flang-rt;compiler-rt" to the flang-rt build, but this seems really pointless and wasteful, because at that point we already have a compiled & working compiler-rt (and failed because it ends up building libcxx, which needs a newer GCC than our current default). I don't get all the involved configuration here, but as I said before:

Not sure at what point the ability of the driver to look into ${prefix}/lib/clang/20/lib/windows gets set, but it sounds like we'd want to force that to always be true.

I don't mind patching clang/flang/whatever for that to be the case, but I'd really like to avoid rebuilding compiler-rt just to nudge the build in that direction.

@Meinersbur
Copy link
Member Author

We are mixing up the Windows issue of finding the right clang_rt.builtins (which is an issue unrelated to this PR) and the UNIX install path of libflang_rt.a. Let's separate these issues.

Windows name of clang_rt.builtins

I added LLVM_ENABLE_RUNTIMES=compiler-rt to building flang, but that didn't change anything. Not sure at what point the ability of the driver to look into ${prefix}/lib/clang/20/lib/windows gets set, but it sounds like we'd want to force that to always be true.

It also needs to be built, i.e.

$ ninja builtins
$ find -name "clang_rt.builtins*.lib"
./lib/clang/20/lib/windows/clang_rt.builtins-x86_64.lib   

Can you confirm that it is there?

I then tried shifting -DLLVM_ENABLE_RUNTIMES="flang-rt;compiler-rt" to the flang-rt build, but this seems really pointless and wasteful, because at that point we already have a compiled & working compiler-rt (and failed because it ends up building libcxx, which needs a newer GCC than our current default). I don't get all the involved configuration here, but as I said before:

I don't get your setup. Why is it compiling libcxx if it is not in LLVM_ENABLE_RUNTIMES? How are you using GCC on Windows? According to https://llvm.org/docs/GettingStarted.html, LLVM does not support being built on Windows using GCC.

Not sure at what point the ability of the driver to look into ${prefix}/lib/clang/20/lib/windows gets set, but it sounds like we'd want to force that to always be true.

I don't mind patching clang/flang/whatever for that to be the case, but I'd really like to avoid rebuilding compiler-rt just to nudge the build in that direction.

This should do it (untested):

diff --git a/clang/lib/Driver/ToolChain.cpp b/clang/lib/Driver/ToolChain.cpp
index 9f174fbda398..b77cbb72e7ec 100644
--- a/clang/lib/Driver/ToolChain.cpp
+++ b/clang/lib/Driver/ToolChain.cpp
@@ -773,7 +773,7 @@ std::string ToolChain::getCompilerRT(const ArgList &Args, StringRef Component,
       buildCompilerRTBasename(Args, Component, Type, /*AddArch=*/true);
   SmallString<128> OldPath(getCompilerRTPath());
   llvm::sys::path::append(OldPath, CRTBasename);
-  if (Path.empty() || getVFS().exists(OldPath))
+  if (Path.empty() || getVFS().exists(OldPath) || getTriple().isWindowsMSVCEnvironment())
     return std::string(OldPath);

   // If none is found, use a file name from the new layout, which may get        

Non-Windows install location of libflang_rt.a

If $PREFIX is CMAKE_INSTALL_DIR, it is the wrong location. it should be in $PREFIX/lib/clang/20/lib/x86_64-unknown-linux-gnu/libflang_rt.a. I don't see how the flang_rt dir could get in there. Can you try with the latest update?

Both flang and flang-rt get installed into the same -DCMAKE_INSTALL_PREFIX=$PREFIX. I did try with the latest state of this PR, and it runs into the same error.

I have no idea what could be going on. Would you mind adding some printf so I can see the result?

diff --git a/flang-rt/CMakeLists.txt b/flang-rt/CMakeLists.txt
index 51dbe6b6cc09..50dac3037ba1 100644
--- a/flang-rt/CMakeLists.txt
+++ b/flang-rt/CMakeLists.txt
@@ -114,12 +114,20 @@ else ()
   extend_path(FLANG_RT_OUTPUT_RESOURCE_LIB_DIR "${FLANG_RT_OUTPUT_RESOURCE_DIR}" "lib${LLVM_LIBDIR_SUFFIX}")
 endif ()
 extend_path(FLANG_RT_INSTALL_RESOURCE_LIB_PATH "${FLANG_RT_INSTALL_RESOURCE_PATH}" "${toolchain_lib_subdir}")
+message("LLVM_TREE_AVAILABLE: ${LLVM_TREE_AVAILABLE}")
+message("toolchain_lib_subdir: ${toolchain_lib_subdir}")
+message("FLANG_RT_OUTPUT_RESOURCE_DIR: ${FLANG_RT_OUTPUT_RESOURCE_DIR}")
+message("FLANG_RT_INSTALL_RESOURCE_PATH: ${FLANG_RT_INSTALL_RESOURCE_PATH}")
+message("FLANG_RT_OUTPUT_RESOURCE_LIB_DIR: ${FLANG_RT_OUTPUT_RESOURCE_LIB_DIR}")
+message("FLANG_RT_INSTALL_RESOURCE_LIB_PATH: ${FLANG_RT_INSTALL_RESOURCE_LIB_PATH}")
 cmake_path(NORMAL_PATH FLANG_RT_OUTPUT_RESOURCE_DIR)
 cmake_path(NORMAL_PATH FLANG_RT_INSTALL_RESOURCE_PATH)
 cmake_path(NORMAL_PATH FLANG_RT_OUTPUT_RESOURCE_LIB_DIR)
 cmake_path(NORMAL_PATH FLANG_RT_INSTALL_RESOURCE_LIB_PATH)

# for all files of Flang-RT.
if (EXISTS "${FLANG_RT_LIBCUDACXX_PATH}/include")
target_include_directories(obj.${name}PTX AFTER PRIVATE "${FLANG_RT_LIBCUDACXX_PATH}/include")
target_compile_definitions(obj.${name}PTX PRIVATE RT_USE_LIBCUDACXX=1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need:

      target_include_directories(${name} AFTER PRIVATE "${FLANG_RT_LIBCUDACXX_PATH}/include")
      target_compile_definitions(${name} PRIVATE RT_USE_LIBCUDACXX=1)

Because the flang_rt.a library itself is also built with CUDA.

Sorry for not catching it earlier, I missed the build warnings from this and only caught this while doing some end to end.

#===------------------------------------------------------------------------===#


add_flangrt_library(CufRuntime STATIC
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change to STATIC only here?

We also currently build and test the share version in our current CUDA Fortran feature development (@clementval).

Can you add SHARED in #121782?

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.