[Graph] Re-add SM110 support, and use Toolkit 12.8 for SM90,100,120#780
Conversation
sm_110 was dropped in #725 because it forced the whole conditional / checkpoint fatbin to be built with CUDA 13.0, which raised the minimum driver version of the bundled sm_120 (RTX 5090) cubin and broke 570-series drivers with CUDA_ERROR_INVALID_IMAGE. Build each SM architecture with the oldest suitable toolkit (sm_110 with CUDA >= 13.0, everything else exactly CUDA 12.8), emit one fatbin per toolkit into the generated header, and pick the loadable one at runtime via GraphManager::load_first_matching_fatbin (a device only loads the fatbin containing its SM). The generation scripts now share scripts/_fatbin_common.py and raise MissingToolkitError if a required toolkit is absent.
Enumerate the shipped NVIDIA archs (sm_90/100/110/120, incl. Thor) in the supported systems list and point users with newer hardware to file an issue. Also fix a stale ARCH_TOOLKIT reference (now ARCH_MIN_TOOLKIT) in the fatbin build guide.
sm_60..sm_120 rather than "including ...", which read as open-ended (e.g. a future sm_130 is not supported).
Parse each committed *_fatbin.h, cuobjdump every bundled blob, and assert the per-toolkit arch split matches group_archs_by_toolkit(SM_VERSIONS): e.g. the condition kernel's sm_90/100/120 in blob 0 (CUDA 12.8) and sm_110 in blob 1 (CUDA 13.0+). Guards the split that keeps the sm_120 cubin loadable on older drivers (genesis-world#2942) and drift-guards SM_VERSIONS vs the headers. Skipped unless a recent-enough cuobjdump is on PATH. Documented under the fatbin build guide.
![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: 90c63c1917
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Replace DEFAULT_TOOLKIT + ARCH_MIN_TOOLKIT with a single SM_TOOLKIT dict that maps every bundled SM to a version spec: "==X.Y" (exact pin) or ">=X.Y" (minimum). This keeps each arch's toolkit requirement explicit and in one place while preserving the exact-vs-minimum semantics (sm_90/100/120 pinned to ==12.8 for wide driver compat; sm_110 needs >=13.0). Unknown SMs now fail fast with a KeyError. Docs updated accordingly.
black wraps two long lines in the fatbin build helper/test; clang-format wraps a signature/comment in graph_manager and rewraps the generated *_fatbin.h byte arrays to the committed 19-per-line layout (byte values unchanged). Note in the fatbin build guide that regenerated headers must be run through pre-commit.
…tbin The yield-check kernel runs on the pre-Hopper flat-graph qd.checkpoint path, so its fatbin must load on pre-Hopper GPUs. Switch its SM list to one target per compute-capability major -- [60, 70, 80, 90, 100, 110, 120] -- which, via CUDA minor-version compatibility, covers Pascal through Blackwell/Thor and matches the advertised >= sm_60 floor. Drops the redundant sm_75/86/89 (already covered by sm_70/sm_80) and adds Pascal/Volta, which previously would have thrown on a yielding checkpoint. Regenerated header + SM_TOOLKIT entries; docs updated.
Hold an explicit EXPECTED_LAYOUT (per blob: expected archs + CUDA toolkit) in the test instead of re-deriving it from scripts/_fatbin_common.py, and assert the build toolkit cuobjdump reports for every embedded cubin. This makes the check independent of the production SM->toolkit mapping and directly pins the #2942 property: sm_120 (RTX 5090) must be built with CUDA 12.8, not a newer toolkit that rejects 570-series drivers with CUDA_ERROR_INVALID_IMAGE.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ceb08698d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
The generated `...Sizes[]` array had no consumer: load_first_matching_fatbin loads via cuModuleLoadData (which reads the length from the fatbin wrapper itself) and the non-empty guard uses `...Count`. It was a leftover from generalizing the old single-fatbin `...Size` scalar. Remove it from the generator and regenerate all three headers. Regenerating also refreshes the embedded nvcc/ptxas build-metadata paths in the condition and gate blobs from the old /usr/local/no_cuda/cuda-12.8 location to the standard /usr/local/cuda-12.8; the SASS is unchanged (yield-check, already regenerated post-move, is byte-identical).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c248b9c9c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
CI enforces no hard-wrapping in .md files, so reflow every paragraph and list item in the CUDA graph conditional fatbin guide to a single line. Also prefix the page title with "Advanced:" (mirroring contributing.md) so the whole page counts as an advanced/internal section -- this clears the doc-quality checker's scope/undefined-term warnings for its contributor-only, jargon-heavy content.
A SASS-only cubin's minimum driver is the CUDA major family floor (12.x -> r525, 13.x -> r580), independent of the minor via minor-version compatibility. So the sm_110 blob built with 13.1 loads on any CUDA 13.0+ (>= r580) driver exactly as a 13.0 build would -- 13.0 is genuinely the minimum. Rewrite the SM_TOOLKIT spec comment to state this accurately (the old "driver CUDA version >= toolkit" phrasing wrongly implied the minor mattered).
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0e805cd50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
An unbounded >=13.0 spec would pick CUDA 14 on a box with only 14.x installed, silently building sm_110 with a newer major and raising its driver floor above the documented r580 (CUDA 13.0) support -- the same "too-new major" failure as genesis-world#2942. Constrain >=X.Y resolution to major X, so a 14-only box now raises MissingToolkitError instead of committing CUDA-14 cubins, and tighten the layout test's "13.0+" matcher to require major 13 (accepts 13.1, rejects 14.0).
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
The doc-quality CI reviews the full contents of any .md the PR touches, so editing supported_systems.md put its pre-existing `qd.simt.subgroup` mention on the hook as an undefined term. Link it to the existing subgroup.md reference.
…nics
The doc-quality CI flagged the AMDGPU note for exposing codegen/backend
mechanics ("codegen always runs in wave64 mode ... overrides it to wave64")
outside an advanced section. Restate the user-observable fact -- subgroups are
64 lanes wide on all AMD GPUs -- without the internal codegen framing.
The doc-quality CI reviews the full file and flagged MoltenVK as an undefined term. Define it inline (Vulkan-to-Metal translation layer), and preemptively drop other specialized jargon that a typical end user would not know: replace "HIP SDK" with "GPU toolchain" and drop the CUDA-runtime link/import-time implementation aside, keeping only the user-facing install requirement.
hughperkins
added
awaiting review pass
|
ok to merge |
2 participants
Issue: #
Brief Summary
Addresses Genesis-Embodied-AI/genesis-world#2942 (comment)
copilot:summary
Walkthrough
copilot:walkthrough