Offload: Build offload as a single Step #150108
Conversation
|
This PR modifies If appropriate, please update This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. This PR modifies If appropriate, please update |
rustbot
added
S-waiting-on-review
|
|
ZuseZ4
force-pushed
the
offload-build-rework
branch
from
93bc6ea to
4b2d2b9
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
ZuseZ4
force-pushed
the
offload-build-rework
branch
from
4b2d2b9 to
dbe2964
Compare
| .join(" ") | ||
| .into(); | ||
|
|
||
| // If we use an external clang as opposed to building our own llvm_clang, than that clang will |
There was a problem hiding this comment.
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.
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.
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?
There was a problem hiding this comment.
Shame that we can't just read the flags from cmake::Config after the function is called, and then modify them in Enzyme/Offload build steps. We already pass ldflags to this function, so adding another CcFlags struct that will contain extra C/CXX flags to add seems like the easiest change.
| } | ||
|
|
||
| impl Step for OmpOffload { | ||
| type Output = PathBuf; |
There was a problem hiding this comment.
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 Assemble step) does not need to guess how are the files named, and the corresponding logic to find the files is localized within this step.
| // 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"); |
There was a problem hiding this comment.
Actually, I think that we should use a different build directory, and handle the copying manually in the Assemble step. We should not rely on other code copying the offload stuff for us. Partly also because we will want to have it as a separate component, and not put the offload things into e.g. the rustc or llvm-tools component by accident.
| .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")); |
There was a problem hiding this comment.
There's a field in LlvmResult with a path to this directory.
| builder.sysroot_target_libdir(target_compiler, target_compiler.host); | ||
| let lib_ext = std::env::consts::DLL_EXTENSION; | ||
|
|
||
| let libenzyme = "libLLVMOffload"; |
There was a problem hiding this comment.
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 Vec<PathBuf>, or as three fields, depending on whether it is useful to distinguish them), and the code here in Assembly should just copy them to the correct directories.
|
@rustbot author |
rustbot
added
S-waiting-on-author
4 participants
r? @Kobzol
Since it looks like we'll postpone enabling offload in CI for a bit, I factored out these improvements which we want independently. I locally tested both build options successfully, the in-tree-clang build, as well as the build where we provide a path to an external clang.