-
-
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?
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 |
|
|
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.
4b2d2b9 to
dbe2964
Compare
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.
Left some comments, otherwise it looks reasonable.
| } | ||
|
|
||
| impl Step for OmpOffload { | ||
| type Output = PathBuf; |
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.
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.
Choose a reason for hiding this comment
The 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 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.
| 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.
Choose a reason for hiding this comment
The 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 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 |
This comment has been minimized.
This comment has been minimized.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
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.