Fix deep-ep sm_80 fatbin build: real cuda_fp8.h + unconditional smem_size in patch - #11
Open
ETOgaosion wants to merge 2 commits into
Open
Fix deep-ep sm_80 fatbin build: real cuda_fp8.h + unconditional smem_size in patch#11ETOgaosion wants to merge 2 commits into
ETOgaosion wants to merge 2 commits into
Conversation
The Ampere fallback in configs.cuh re-declares __nv_fp8_interpretation_t / __nv_fp8x4_e4m3 / __nv_fp8_storage_t as int/uint8_t. During the sm_80 device pass, torch/nvshmem headers pull the toolkit <cuda_fp8.h> in transitively, causing "invalid redeclaration" errors (71 errors in intranode.cu, CUDA 13.0). This path was never compiled before: pre-verl-project#10 skip logic skipped the x86_64 deep-ep build, so the sm_80 cubin was never exercised in CI. No sm_80 compilation unit references FP8 symbols (the only FP8 call sites are in internode_ll.cu, whose sm_80 cubin is compiled out by the same patch), so configs.cuh now unconditionally includes <cuda_fp8.h> like the host / sm_90 / sm_100 passes already did.
The runtime-dispatched SET_SHARED_MEMORY_FOR_TMA macro (unlike upstream's Ampere build, where it #defines to void()) always expands to code that references the caller's smem_size, but intranode.cu declares that constant inside #ifndef DISABLE_SM90_FEATURES. In the sm_80 device pass the snippet defines DISABLE_SM90_FEATURES, so the declaration vanishes and the launch macro expansion fails with "identifier smem_size is undefined" at intranode.cu:521 and :941. Move both smem_size declarations out of the guard (dispatch and combine launch wrappers); the cudaFuncSetAttribute call stays behind the runtime __use_sm90_launch check. Patch fails loudly unless exactly the two expected guarded declarations are present.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix deep-ep x86_64 sm_80 fatbin build (two patch bugs exposed by the #10 full rebuild)
After #10 merged, the fail-closed manifest logic forced a full deep-ep rebuild and the x86_64 wheel failed compiling
csrc/kernels/intranode.cufor the sm_80 gencode. This path was never compiled before #10: the old skip logic skipped the x86_64 deep-ep build because an sm_90-only wheel already existed in the release, so the sm_80 cubin added by #8 was never actually exercised in CI. (aarch64 9.0;10.0 built green.)Both bugs are in the build-time-only patch
ci/patches/enable_deep_ep_sm80.py; the DeepEP submodule is untouched.Fix 1 — fake FP8 typedefs collide with the real
cuda_fp8.h(commit c97733c)Errors (71) in the sm_80 device pass:
The sm_80-only
DISABLE_SM90_FEATURESdefine makes upstreamconfigs.cuhtake its Ampere#elsebranch, which re-declares the FP8 API with fake typedefs (typedef int __nv_fp8_interpretation_t;,typedef uint8_t __nv_fp8_storage_t;,#define __NV_E4M3 0). In the same TU, torch/nvshmem headers transitively pull in the toolkit's real<cuda_fp8.h>→ redeclaration / type-mismatch.Fix:
configs.cuhnow unconditionally includes the real<cuda_fp8.h>. No sm_80 compilation unit references any FP8 symbol — the only FP8 call sites are ininternode_ll.cu, whose sm_80 cubin is compiled out by this same patch (verified by grepping every sm_80-compiled TU;calculate_fp8_scalesis pure float math). Host / sm_90 / sm_100 passes already used the real header.Fix 2 —
smem_sizeundefined at the launch macro sites (commit e65a9d1)First branch CI run (33861703220) then failed with exactly:
(and zero FP8 errors — fix 1 confirmed).
intranode.cudeclaressmem_sizeinside#ifndef DISABLE_SM90_FEATURES(dispatch + combine launch wrappers). Upstream's Ampere build definesSET_SHARED_MEMORY_FOR_TMA(kernel)tovoid()under that guard, so the missing constant never mattered; our runtime-dispatched macro (edit 3 of the patch) always expands to code referencingsmem_size(thecudaFuncSetAttributecall is a runtime Hopper+ check). Fix: move bothsmem_sizeconstants out of the guard; the patch fails loudly unless exactly the two expected guarded declarations are present.Other
DISABLE_SM90_FEATURESsites in sm_80-compiled TUs were audited: the 6 remaining guards inintranode.cuand the 2 inutils.cuhare device-kernel TMA/elect paths with upstream's own Ampere#elsefallbacks;runtime.cu/layout.cuhave no guards and noSET_SHARED_MEMORY_FOR_TMAcalls;internode.cu/internode_ll.cu/pcie.cuare compiled out for sm_80 entirely.Verification
#include <cuda_fp8.h>, zero fake FP8 typedefs, and 6 (was 8) SM90 guards inintranode.cu.python -m unittest test_cuda_archs test_generate_matrix— 43/43 pass.smem_sizesites remained.build_inputsfingerprint, so the deep-ep skip decision correctly flips to rebuild after merge.