Repository navigation
Conversation
d3bc84e to
6b88325
Compare
Generate Linux and Windows declarations for each CUDA version using the redistributable implementation labels. Preserve platform and host-default registration aliases, including Windows x86_64 constraints. Extend cross-major integration coverage to both OS declarations, host aliases, default version selection, and RDC builds.
6b88325 to
5aaa26e
Compare
Remove toolkit_versions and the attribute-level select generation introduced by #490. Each implementation now has a fixed CUDA version, selected through the versioned toolchain facade. Retain the version-gate regression test and validate it alongside cross-major integration on Bazel 7 and 9.
|
@janickm Commit 2c3cce6 removes the attribute-level version selection introduced in #490, while keeping its regression tests. #490 fixes the reported toolchain version, but repository generation and BUILD-file evaluation can still use the maximum declared version. Selecting individual attributes therefore leaves some version-dependent decisions inconsistent. This PR instead generates a separate toolchain implementation for each CUDA release, with its component references and version metadata fixed to that release. cuda:version selects the matching implementation, addressing #477. Each component repository also gets its own defs.bzl, stamped with the containing toolkit’s version. This makes if_cuda_toolkit_version_ge evaluate against the correct release, fixing #497 and allowing #496’s version checks to work correctly with multiple declared toolkits. |
janickm
left a comment
janickm
left a comment
There was a problem hiding this comment.
Took a look, and I think this is the right call - agreed that selecting individual attributes was the weaker fix. It only corrects what the toolchain reports; repository generation and load-time BUILD evaluation still saw the maximum version, so the inconsistency just moved somewhere less visible. Generating a separate implementation per release and selecting it through toolchain resolution addresses the cause rather than one symptom, and no objection at all to #490's implementation coming out - the mechanism here supersedes it cleanly.
Verified locally against 2c3cce6, declaring 13.0.2 and 12.8.1:
Per-component defs.bzl (the #497 fix). Each component repo is now stamped with its own release, and if_cuda_toolkit_version_ge evaluates against it:
cuda_cccl_linux_x86_64_12_8_1/defs.bzl: ctk_version = (12, 8)
cuda_cccl_linux_x86_64_13_0_2/defs.bzl: ctk_version = (13, 0)
so the CCCL include paths come out right in both, where on main the 12.8.1 repo wrongly resolved the CUDA 13 layout:
| repo | thrust includes |
on main |
|---|---|---|
..._12_8_1 |
cccl/include |
cccl/include/cccl (wrong) |
..._13_0_2 |
cccl/include/cccl |
same |
Version gate still behaves. The flag follows the nvcc that actually runs:
cuda:version |
nvcc | --frandom-seed |
|---|---|---|
| 12.9.1 | cuda_nvcc_..._12_9_1 |
present |
| 12.8.1 | cuda_nvcc_..._12_8_1 |
absent |
Builds pass at each version and with the flag unset, and the new negative case fails with No matching toolchains found as asserted. Nice addition - it pins down that an undeclared version fails loudly rather than silently falling back.
Backward compatibility is the thing I was most wary of, and it holds: nvcc-local-toolchain, clang-local-toolchain and disabled-local-toolchain are all still declared by the facade, so existing register_toolchains lists keep resolving even though the module now uses the @cuda//toolchain/... wildcard. A single declared version also goes through the facade now, and WORKSPACE mode is unaffected.
Also ran: bazel test //tests/... (93 pass, 6 skipped), the examples including //rdc:all since the device-link path reads version_major, test_all.sh end to end (exit 0, including the new cross-major case), and pre-commit.
Nothing blocking from me. One question, only because it changes what I do next in #496: with each component repo carrying its own defs.bzl, the if_local_cuda_toolkit guard I put on the new cccl_headers target may be unnecessary - it was there because the fragment is also expanded into @cuda itself for a local toolkit, where the component name is cuda and BUILD.lctk_cuda already owns cuda_headers. I'll rebase #496 on this once it lands and re-check whether the guard can go; happy to do that now against this branch instead if you'd rather review them together.
| _patch_nvvm(repository_ctx, component_name) | ||
|
|
||
| toolkit_version = repository_ctx.attr.toolkit_version or repository_ctx.attr.version | ||
| defs_label = "//:defs.bzl" if toolkit_version else "@cuda//:defs.bzl" |
There was a problem hiding this comment.
Is it possible to unify to @cuda//:defs.bzl so that we can get rid of this var.
| repository_ctx, | ||
| libpath = "lib", | ||
| components = {component_name: repository_ctx.name}, | ||
| defs_label = defs_label, |
| load("@cuda//:defs.bzl", "additional_header_deps", "any_or_first", "if_cuda_toolkit_version_ge", "if_local_cuda_toolkit") # @unused | ||
| load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_import", "cc_library") | ||
| load("@rules_cuda//cuda:defs.bzl", "cc_import_versioned_sos", "if_linux", "if_windows") # @unused | ||
| load("%{defs_label}", "additional_header_deps", "any_or_first", "if_cuda_toolkit_version_ge", "if_local_cuda_toolkit") # @unused |
| "@cuda//toolchain:nvcc-local-toolchain", | ||
| "@cuda//toolchain/clang:clang-local-toolchain", | ||
| "@cuda//toolchain/disabled:disabled-local-toolchain", | ||
| "@cuda//toolchain/...", |
There was a problem hiding this comment.
What happens if ... gives any non-toolchain target?
There was a problem hiding this comment.
Why this new file? Can't we just use existing without _nvvm_embedded?
| if not component_platforms: | ||
| continue | ||
|
|
||
| if len(component_platforms) == 1: |
There was a problem hiding this comment.
| if not component_platforms: | |
| continue | |
| if len(component_platforms) == 1: | |
| if len(component_platforms) == 0: | |
| continue | |
| elif len(component_platforms) == 1: |
| cuda_toolkit( | ||
| name = toolchain_repo_name, | ||
| components_mapping = exact_components_mapping, | ||
| version = redist_version, | ||
| ) |
There was a problem hiding this comment.
This will generate multiple repos for cuda with version. Can we document it somewhere about the hierarchy. See #477 (comment), extend the mermaid diagram and extend it, put it somewhere in the document with explanation.
Remove defs_label from BUILD generation. Keep release-specific helpers in versioned component repositories and inherit the toolkit helpers for unversioned manual components.
Use BUILD.nvvm for embedded NVVM files in pre-13 nvcc archives and retain dummy aliases for CUDA 13 nvcc repositories. Remove the duplicate template while preserving local toolkit target ownership.
Document that Bazel filters recursive target patterns to toolchain rules, excluding implementation rules, config settings, and aliases. Explicit non-toolchain registrations remain errors.
Extend the component alias diagram with per-release toolchain implementations, platform selection, and repository-local version helpers. Publish the page in the developer documentation and enable Mermaid rendering.
This branch has not been deployed
2 participants
No description provided.