Offload cmake cleanups - #162196
Offload cmake cleanups#162196ZuseZ4 wants to merge 2 commits into
Conversation
|
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. |
rustbot
added
A-rustc-dev-guide
|
|
This comment has been minimized.
This comment has been minimized.
9d399d1 to
341d294
Compare
| /// | ||
| /// Deliberately not under `llvm_output_dir`, since running cmake twice in the same folder is | ||
| /// known to cause issues, like deleting existing binaries. | ||
| pub fn offload_out(&self, target: TargetSelection) -> PathBuf { |
There was a problem hiding this comment.
I deliberately removed these functions recently, because they are an antipattern 😅 Steps should be ensuring other steps to get the build directory, rather than implicitly depending on paths from a shared function.
| @@ -2899,7 +2898,7 @@ impl CommandLineStep for Offload { | |||
| tarball.set_overlay(OverlayKind::Offload); | |||
| tarball.is_preview(true); | |||
|
|
|||
| let omp_offload_libdir = builder.out.join(target).join("offload").join("lib"); | |||
| let omp_offload_libdir = builder.offload_out(target).join("lib"); | |||
There was a problem hiding this comment.
This should be using the path from the omp_offload step output.
| // binaries. We therefore write our offload artifacts into it's own folder, instead of | ||
| // using the llvm build dir. | ||
| let out_dir = builder.out.join(self.target.triple).join("offload"); | ||
| let out_dir = builder.offload_out(self.target); |
There was a problem hiding this comment.
This should hardcode the path here, this is the canonical place where that path is decided.
|
@rustbot author |
rustbot
added
S-waiting-on-author
341d294 to
808b60c
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.
Originally introduced in the Enzyme build and coppied around since, but without effect for a few years by now.
808b60c to
d5a0a16
Compare
4 participants
r? kobzol
Split out of #161565, which requires (minor) LLVM changes.
These cleanups are valuable on their own already, and shouldn't be blocked. So let's land it, before it get's stale with all the refactoring going on ^^
Copied the disclaimer: I used an llm to implement the fixes, since I still don't like cmake.
I confirm that on a high-level these are the right fixes as far as I can tell, and I reviewed/refactored it.