Better cuda cache - #255
Conversation
Keep the clang-dumper includes archive limited to built-in libc/libc++ and configure the system Clang resource directory for SYSTEM-mode CUDA parsing. Built-in CUDA supplies only the NVIDIA cudalib. Select a host-compatible CUDA platform from the intersection of all required NVIDIA manifest components, then reuse that parsed manifest for installation. Tests: CudaResourcesTest, ClangResourcesTest, and CxxCudaTest
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5d4da0a1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var systemResourceDir = libcMode == LibcMode.SYSTEM && useBuiltinCuda | ||
| ? prepareSystemClangResourceDir(manifest) | ||
| : null; |
There was a problem hiding this comment.
When CUDA_PATH is <builtin> and libc mode is SYSTEM, this now unconditionally searches for a matching clang++-<llvmMajor> installation and throws before parsing if it is absent. The downloaded clang-dumper otherwise supports system libc environments that only have GCC, and before this change built-in CUDA caused the bundled includes asset (including Clang's CUDA wrapper headers) to be prepared, so this common configuration did not require a separately installed Clang 18. Please retain or fall back to the bundled Clang resource headers rather than making system Clang an additional prerequisite.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces substantial new download/install/cache/locking behavior and changes resource resolution paths, which warrants careful human validation across supported platforms and environments.
Pull request overview
This PR reworks Clava’s built-in CUDA support to use NVIDIA redistribution packages (downloaded and cached per CUDA release/platform) and reorganizes the cache layout so Clang dumper and CUDA resources share a common base cache directory but remain namespaced.
Changes:
- Introduces
CudaResourcesto download/validate/assemble CUDA redistribution packages and manage a per-release cache with cleanup/locking. - Updates
ClangResources/parser pipeline to support built-in CUDA withSYSTEMlibc by optionally using a system Clang-resource-dir. - Adjusts cache root semantics (JS side-effects,
CodeParser.DUMPER_FOLDERlabel, cache folder structure) and extends cache install verification with expected-size checking.
File summaries
| File | Description |
|---|---|
| Clava-JS/code/sideEffects.ts | Sets CodeParser.DUMPER_FOLDER to a shared Clava cache root (instead of clang-dumper-only). |
| ClangAstParser/test/pt/up/fe/specs/clang/parser/tests/CxxCudaTest.java | Updates CUDA parsing test to run with BUILTIN_AND_LIBC. |
| ClangAstParser/test/pt/up/fe/specs/clang/parser/CxxCudaTester.java | Gates CUDA tests on built-in CUDA support via ClangResources. |
| ClangAstParser/test/pt/up/fe/specs/clang/CudaResourcesTest.java | Adds comprehensive unit tests for manifest parsing, installation, extraction safety, concurrency, and cleanup. |
| ClangAstParser/test/pt/up/fe/specs/clang/ClangResourcesTest.java | Updates tests to reflect new cache layout and SYSTEM-libc + builtin-CUDA behavior. |
| ClangAstParser/src/pt/up/fe/specs/clang/dumper/ClangAstDumper.java | Adds optional -resource-dir support for system Clang resource directory usage. |
| ClangAstParser/src/pt/up/fe/specs/clang/CudaResources.java | Implements CUDA redistrib download, validation, assembly, locking, and stale-cache cleanup. |
| ClangAstParser/src/pt/up/fe/specs/clang/codeparser/ParallelCodeParser.java | Propagates libc mode into per-parse options and passes systemResourceDir into the dumper. |
| ClangAstParser/src/pt/up/fe/specs/clang/codeparser/CodeParser.java | Updates DUMPER_FOLDER label to reflect broader cache responsibility (dumper + CUDA). |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangResources.java | Moves clang-dumper cache under clang-dumper/ and integrates builtin CUDA + system resource dir logic. |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangFiles.java | Extends ClangFiles record to include systemResourceDir. |
| ClangAstParser/src/pt/up/fe/specs/clang/ClangAstWebResource.java | Adds cuda-release.tag support and removes legacy cudalib zip handling. |
| ClangAstParser/src/pt/up/fe/specs/clang/CacheFiles.java | Adds optional expected-size verification to installFile. |
| ClangAstParser/cuda-release.tag | New CUDA release tag resource (used to select redistrib manifest). |
| ClangAstParser/clang-dumper-release.tag | Bumps clang-dumper release tag. |
| ClangAstParser/build.gradle | Adds commons-compress + xz deps and includes cuda-release.tag as a resource. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public static boolean isBuiltinCudaSupported() { | ||
| return CudaResources.isSupportedPlatform(); | ||
| } |
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
No description provided.