-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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 |
|---|---|---|
|
|
@@ -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 | ||
| // 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::create_dir_all(&out_dir).unwrap(); | ||
| } | ||
|
|
||
| // 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, | ||
|
|
||
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.
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.
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.
Now that you mention it, it really does. Maybe we specify it, or the main cmake is more cautious than the offload cmake and overwrites include dirs. Or they are close enough that it didn't matter so far, but I'm not really convinced by that explanation if even the much smaller offload project ran into issues.
Unrelatedly, I noticed that this currently affects how we build LLVM, I only wanted to affect how we build offload.
Since I have a similar issue with Enzyme (where I only want to ignore deprecation warnings for enzyme, not for llvm builds) I'll modify this function to accept extra flags.
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.
So, I need to make sure that my logic is only applied when building offload or enzyme, not when building llvm.
Shall I search some of the paths given to this function for containing keywords like enzyme to decide, or shall I add extra args to this function?
The right thing would be to add to cxxflags from the corresponding step, but as you can see in the comment there, we ignore our cflags/cxxflags variables due to an llvm bug. Any preference?
Uh oh!
There was an error while loading. Please reload this page.
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.
Shame that we can't just read the flags from
cmake::Configafter the function is called, and then modify them in Enzyme/Offload build steps. We already passldflagsto this function, so adding anotherCcFlagsstruct that will contain extra C/CXX flags to add seems like the easiest change.