-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
Offload: Build offload as a single Step #150108
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1427,10 +1427,12 @@ fn rustc_llvm_env(builder: &Builder<'_>, cargo: &mut Cargo, target: TargetSelect | |||||
| if builder.config.llvm_enzyme { | ||||||
| cargo.env("LLVM_ENZYME", "1"); | ||||||
| } | ||||||
| let llvm::LlvmResult { host_llvm_config, .. } = builder.ensure(llvm::Llvm { target }); | ||||||
| if builder.config.llvm_offload { | ||||||
| builder.ensure(llvm::OmpOffload { target }); | ||||||
| cargo.env("LLVM_OFFLOAD", "1"); | ||||||
| } | ||||||
| let llvm::LlvmResult { host_llvm_config, .. } = builder.ensure(llvm::Llvm { target }); | ||||||
|
|
||||||
| cargo.env("LLVM_CONFIG", &host_llvm_config); | ||||||
|
|
||||||
| // Some LLVM linker flags (-L and -l) may be needed to link `rustc_llvm`. Its build script | ||||||
|
|
@@ -2293,6 +2295,45 @@ impl Step for Assemble { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| if builder.config.llvm_offload && !builder.config.dry_run() { | ||||||
| debug!("`llvm_offload` requested"); | ||||||
| let offload_install = builder.ensure(llvm::OmpOffload { target: build_compiler.host }); | ||||||
| if let Some(_llvm_config) = builder.llvm_config(builder.config.host_target) { | ||||||
| let src_dir = offload_install.join("lib"); | ||||||
| let libdir = builder.sysroot_target_libdir(build_compiler, build_compiler.host); | ||||||
| let target_libdir = | ||||||
| builder.sysroot_target_libdir(target_compiler, target_compiler.host); | ||||||
| let lib_ext = std::env::consts::DLL_EXTENSION; | ||||||
|
|
||||||
| let libenzyme = "libLLVMOffload"; | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted in another comment, the logic for locating these components should ideally be localized within the offload step itself. It should then contain the paths to the dylibs (either just as a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| let src_lib = src_dir.join(libenzyme).with_extension(lib_ext); | ||||||
| let dst_lib = libdir.join(libenzyme).with_extension(lib_ext); | ||||||
| let target_dst_lib = target_libdir.join(libenzyme).with_extension(lib_ext); | ||||||
| builder.resolve_symlink_and_copy(&src_lib, &dst_lib); | ||||||
| builder.resolve_symlink_and_copy(&src_lib, &target_dst_lib); | ||||||
|
|
||||||
| // FIXME(offload): With LLVM-22, we should be able to drop everything below here. | ||||||
| let omp = "libomp"; | ||||||
| let src_omp = src_dir.join(omp).with_extension(lib_ext); | ||||||
| let dst_omp_lib = libdir.join(omp).with_extension(lib_ext); | ||||||
| let target_omp_dst_lib = target_libdir.join(omp).with_extension(lib_ext); | ||||||
| builder.resolve_symlink_and_copy(&src_omp, &dst_omp_lib); | ||||||
| builder.resolve_symlink_and_copy(&src_omp, &target_omp_dst_lib); | ||||||
|
|
||||||
| let tgt = "libomptarget"; | ||||||
| let src_tgt = src_dir.join(tgt).with_extension(lib_ext); | ||||||
| let dst_tgt_lib = libdir.join(tgt).with_extension(lib_ext); | ||||||
| let target_tgt_dst_lib = target_libdir.join(tgt).with_extension(lib_ext); | ||||||
| builder.resolve_symlink_and_copy(&src_tgt, &dst_tgt_lib); | ||||||
| builder.resolve_symlink_and_copy(&src_tgt, &target_tgt_dst_lib); | ||||||
|
|
||||||
| // FIXME(offload): Add amdgcn-amd-amdhsa and nvptx64-nvidia-cuda folder | ||||||
| // This one is slightly more tricky, since we have the same file twice, in two | ||||||
| // subfolders for amdgcn and nvptx64. We'll likely find two more in the future, once | ||||||
| // Intel and Spir-V support lands in offload. | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Build the libraries for this compiler to link to (i.e., the libraries | ||||||
| // it uses at runtime). | ||||||
| debug!( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -454,16 +454,6 @@ impl Step for Llvm { | |
| enabled_llvm_runtimes.push("compiler-rt"); | ||
| } | ||
|
|
||
| // This is an experimental flag, which likely builds more than necessary. | ||
| // We will optimize it when we get closer to releasing it on nightly. | ||
| if builder.config.llvm_offload { | ||
| enabled_llvm_runtimes.push("offload"); | ||
| //FIXME(ZuseZ4): LLVM intends to drop the offload dependency on openmp. | ||
| //Remove this line once they achieved it. | ||
| enabled_llvm_runtimes.push("openmp"); | ||
| enabled_llvm_projects.push("compiler-rt"); | ||
| } | ||
|
|
||
| if !enabled_llvm_projects.is_empty() { | ||
| enabled_llvm_projects.sort(); | ||
| enabled_llvm_projects.dedup(); | ||
|
|
@@ -778,6 +768,17 @@ fn configure_cmake( | |
| .collect::<Vec<String>>() | ||
| .join(" ") | ||
| .into(); | ||
|
|
||
| // If we use an external clang as opposed to building our own llvm_clang, than that clang will | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does that mean that even if we don't enable offload, the host Clang will somehow use its own include directories when building LLVM? That sounds suspicious. |
||
| // come with it's own set of default include directories, which are based on a potentially older | ||
| // LLVM. This can cause issues, so we overwrite it to include headers based on our | ||
| // `src/llvm-project` submodule instead. | ||
| // FIXME(offload): With LLVM-22 we hopefully won't need an external clang anymore. | ||
| let base = builder.llvm_out(target).join("include"); | ||
| let inc_dir = base.display(); | ||
| if builder.config.llvm_offload && !builder.config.llvm_clang { | ||
| cflags.push(format!(" -I {inc_dir}")); | ||
| } | ||
| if let Some(ref s) = builder.config.llvm_cflags { | ||
| cflags.push(" "); | ||
| cflags.push(s); | ||
|
|
@@ -789,6 +790,7 @@ fn configure_cmake( | |
| cflags.push(format!(" --target={target}")); | ||
| } | ||
| cfg.define("CMAKE_C_FLAGS", cflags); | ||
|
|
||
| let mut cxxflags: OsString = builder | ||
| .cc_handled_clags(target, CLang::Cxx) | ||
| .into_iter() | ||
|
|
@@ -801,6 +803,9 @@ fn configure_cmake( | |
| .collect::<Vec<String>>() | ||
| .join(" ") | ||
| .into(); | ||
| if builder.config.llvm_offload && !builder.config.llvm_clang { | ||
| cxxflags.push(format!(" -I {inc_dir}")); | ||
| } | ||
| if let Some(ref s) = builder.config.llvm_cxxflags { | ||
| cxxflags.push(" "); | ||
| cxxflags.push(s); | ||
|
|
@@ -896,6 +901,133 @@ fn get_var(var_base: &str, host: &str, target: &str) -> Option<OsString> { | |
| .or_else(|| env::var_os(var_base)) | ||
| } | ||
|
|
||
| // FIXME(offload): In an ideal world, we would just enable the offload runtime in our previous LLVM | ||
| // build step. For now, we still depend on the openmp runtime since we use some of it's API, so we | ||
| // build both. However, when building those runtimes as part of the LLVM step, then LLVM's cmake | ||
| // implicitly assumes that Clang has also been build and will try to use it. In the Rust CI, we | ||
| // don't always build clang (due to compile times), but instead use a slightly older external clang. | ||
| // LLVM tries to remove this build dependency of offload/openmp on Clang for LLVM-22, so in the | ||
| // future we might be able to integrate this step into the LLVM step. For now, we instead introduce | ||
| // a Clang_DIR bootstrap option, which allows us tell CMake to use an external clang for these two | ||
| // runtimes. This external clang will try to use it's own (older) include dirs when building our | ||
| // in-tree LLVM submodule, which will cause build failures. To prevent those, we now also | ||
| // explicitly set our include dirs. | ||
| #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] | ||
| pub struct OmpOffload { | ||
| pub target: TargetSelection, | ||
| } | ||
|
|
||
| impl Step for OmpOffload { | ||
| type Output = PathBuf; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please modify this to be more similar to #150071? So that it returns some named struct that contains the final path to the libXXX dylibs. So that code that then uses the output (e.g. in the |
||
| const IS_HOST: bool = true; | ||
|
|
||
| fn should_run(run: ShouldRun<'_>) -> ShouldRun<'_> { | ||
| run.path("src/llvm-project/offload") | ||
| } | ||
|
|
||
| fn make_run(run: RunConfig<'_>) { | ||
| run.builder.ensure(OmpOffload { target: run.target }); | ||
| } | ||
|
|
||
| /// Compile OpenMP offload runtimes for `target`. | ||
| #[allow(unused)] | ||
| fn run(self, builder: &Builder<'_>) -> PathBuf { | ||
| if builder.config.dry_run() { | ||
| return builder.config.tempdir().join("llvm-offload-dry-run"); | ||
| } | ||
| let target = self.target; | ||
|
|
||
| let LlvmResult { host_llvm_config, .. } = builder.ensure(Llvm { target: self.target }); | ||
|
|
||
| // Running cmake twice in the same folder is known to cause issues, like deleting existing | ||
| // binaries. We therefore write our offload artifacts into it's own subfolder. We use a | ||
| // subfolder, so that all the logic that processes our build artifacts (hopefully) also | ||
| // automatically manages our artifacts in the subfolder. | ||
| let out_dir = builder.llvm_out(target).join("offload-outdir"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I think that we should use a different build directory, and handle the copying manually in the |
||
| if std::fs::exists(&out_dir).is_ok_and(|x| !x) { | ||
| std::fs::DirBuilder::new().create(&out_dir).unwrap(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lol, I never saw this in the stdlib, TIL. Isn't it enough to do |
||
| } | ||
|
|
||
| // Offload/OpenMP are just subfolders of LLVM, so we can use the LLVM sha. | ||
| static STAMP_HASH_MEMO: OnceLock<String> = OnceLock::new(); | ||
| let smart_stamp_hash = STAMP_HASH_MEMO.get_or_init(|| { | ||
| generate_smart_stamp_hash( | ||
| builder, | ||
| &builder.config.src.join("src/llvm-project/offload"), | ||
| builder.in_tree_llvm_info.sha().unwrap_or_default(), | ||
| ) | ||
| }); | ||
| let stamp = BuildStamp::new(&out_dir).with_prefix("offload").add_stamp(smart_stamp_hash); | ||
|
|
||
| trace!("checking build stamp to see if we need to rebuild offload/openmp artifacts"); | ||
| if stamp.is_up_to_date() { | ||
| trace!(?out_dir, "offload/openmp build artifacts are up to date"); | ||
| if stamp.stamp().is_empty() { | ||
| builder.info( | ||
| "Could not determine the Offload submodule commit hash. \ | ||
| Assuming that an Offload rebuild is not necessary.", | ||
| ); | ||
| builder.info(&format!( | ||
| "To force Offload/OpenMP to rebuild, remove the file `{}`", | ||
| stamp.path().display() | ||
| )); | ||
| } | ||
| return out_dir; | ||
| } | ||
|
|
||
| trace!(?target, "(re)building offload/openmp artifacts"); | ||
| builder.info(&format!("Building OpenMP/Offload for {target}")); | ||
| t!(stamp.remove()); | ||
| let _time = helpers::timeit(builder); | ||
| t!(fs::create_dir_all(&out_dir)); | ||
|
|
||
| builder.config.update_submodule("src/llvm-project"); | ||
| let mut cfg = cmake::Config::new(builder.src.join("src/llvm-project/runtimes/")); | ||
| configure_cmake(builder, target, &mut cfg, true, LdFlags::default(), &[]); | ||
|
|
||
| // Re-use the same flags as llvm to control the level of debug information | ||
| // generated for offload. | ||
| let profile = match (builder.config.llvm_optimize, builder.config.llvm_release_debuginfo) { | ||
| (false, _) => "Debug", | ||
| (true, false) => "Release", | ||
| (true, true) => "RelWithDebInfo", | ||
| }; | ||
| trace!(?profile); | ||
|
|
||
| // OpenMP/Offload builds currently (LLVM-21) still depend on Clang, although there are | ||
| // intentions to loosen this requirement for LLVM-22. If we were to | ||
| let clang_dir = if !builder.config.llvm_clang { | ||
| // We must have an external clang to use. | ||
| assert!(&builder.build.config.llvm_clang_dir.is_some()); | ||
| builder.build.config.llvm_clang_dir.clone() | ||
| } else { | ||
| // No need to specify it, since we use the in-tree clang | ||
| None | ||
| }; | ||
|
|
||
| // FIXME(offload): Once we move from OMP to Offload (Ol) APIs, we should drop the openmp | ||
| // runtime to simplify our build. We should also re-evaluate the LLVM_Root and try to get | ||
| // rid of the Clang_DIR, once we upgrade to LLVM-22. | ||
| cfg.out_dir(&out_dir) | ||
| .profile(profile) | ||
| .env("LLVM_CONFIG_REAL", &host_llvm_config) | ||
| .define("LLVM_ENABLE_ASSERTIONS", "ON") | ||
| .define("LLVM_ENABLE_RUNTIMES", "openmp;offload") | ||
| .define("LLVM_INCLUDE_TESTS", "OFF") | ||
| .define("OFFLOAD_INCLUDE_TESTS", "OFF") | ||
| .define("OPENMP_STANDALONE_BUILD", "ON") | ||
| .define("LLVM_ROOT", builder.llvm_out(target).join("build")) | ||
| .define("LLVM_DIR", builder.llvm_out(target).join("lib").join("cmake").join("llvm")); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's a field in |
||
| if let Some(p) = clang_dir { | ||
| cfg.define("Clang_DIR", p); | ||
| } | ||
| cfg.build(); | ||
|
|
||
| t!(stamp.write()); | ||
| out_dir | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)] | ||
| pub struct Enzyme { | ||
| pub target: TargetSelection, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,6 +169,7 @@ pub struct Config { | |
| pub llvm_link_jobs: Option<u32>, | ||
| pub llvm_version_suffix: Option<String>, | ||
| pub llvm_use_linker: Option<String>, | ||
| pub llvm_clang_dir: Option<PathBuf>, | ||
| pub llvm_allow_old_toolchain: bool, | ||
| pub llvm_polly: bool, | ||
| pub llvm_clang: bool, | ||
|
|
@@ -291,6 +292,7 @@ pub struct Config { | |
| pub miri_info: channel::GitInfo, | ||
| pub rustfmt_info: channel::GitInfo, | ||
| pub enzyme_info: channel::GitInfo, | ||
| pub offload_info: channel::GitInfo, | ||
| pub in_tree_llvm_info: channel::GitInfo, | ||
| pub in_tree_gcc_info: channel::GitInfo, | ||
|
|
||
|
|
@@ -602,6 +604,7 @@ impl Config { | |
| ldflags: llvm_ldflags, | ||
| use_libcxx: llvm_use_libcxx, | ||
| use_linker: llvm_use_linker, | ||
| clang_dir: llvm_clang_dir, | ||
| allow_old_toolchain: llvm_allow_old_toolchain, | ||
| offload: llvm_offload, | ||
| polly: llvm_polly, | ||
|
|
@@ -1261,6 +1264,8 @@ impl Config { | |
| let in_tree_gcc_info = git_info(&exec_ctx, false, &src.join("src/gcc")); | ||
| let in_tree_llvm_info = git_info(&exec_ctx, false, &src.join("src/llvm-project")); | ||
| let enzyme_info = git_info(&exec_ctx, omit_git_hash, &src.join("src/tools/enzyme")); | ||
| let offload_info = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed, the commit will be always the same as the LLVM commit. See #148671 (comment). |
||
| git_info(&exec_ctx, omit_git_hash, &src.join("src/llvm-project/offload")); | ||
| let miri_info = git_info(&exec_ctx, omit_git_hash, &src.join("src/tools/miri")); | ||
| let rust_analyzer_info = | ||
| git_info(&exec_ctx, omit_git_hash, &src.join("src/tools/rust-analyzer")); | ||
|
|
@@ -1361,6 +1366,7 @@ impl Config { | |
| llvm_cflags, | ||
| llvm_clang: llvm_clang.unwrap_or(false), | ||
| llvm_clang_cl, | ||
| llvm_clang_dir: llvm_clang_dir.map(PathBuf::from), | ||
| llvm_cxxflags, | ||
| llvm_enable_warnings: llvm_enable_warnings.unwrap_or(false), | ||
| llvm_enzyme: llvm_enzyme.unwrap_or(false), | ||
|
|
@@ -1401,6 +1407,7 @@ impl Config { | |
| musl_root: rust_musl_root.map(PathBuf::from), | ||
| ninja_in_file: llvm_ninja.unwrap_or(true), | ||
| nodejs: build_nodejs.map(PathBuf::from), | ||
| offload_info, | ||
| omit_git_hash, | ||
| on_fail: flags_on_fail, | ||
| optimized_compiler_builtins, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -32,6 +32,7 @@ define_config! { | |||||
| ldflags: Option<String> = "ldflags", | ||||||
| use_libcxx: Option<bool> = "use-libcxx", | ||||||
| use_linker: Option<String> = "use-linker", | ||||||
| clang_dir: Option<String> = "offload-clang-dir", | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| allow_old_toolchain: Option<bool> = "allow-old-toolchain", | ||||||
| offload: Option<bool> = "offload", | ||||||
| polly: Option<bool> = "polly", | ||||||
|
|
@@ -110,6 +111,7 @@ pub fn check_incompatible_options_for_ci_llvm( | |||||
| ldflags, | ||||||
| use_libcxx, | ||||||
| use_linker, | ||||||
| clang_dir: _, | ||||||
| allow_old_toolchain, | ||||||
| offload, | ||||||
| polly, | ||||||
|
|
||||||
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.
It's a bit weird that it is called
dir, but the comment states that it should be a path to a file. Should it be a path to a directory that contains this file?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.
yes, it's the dir in which the file is.
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.
Ok, then please update the comment :)