Conversation
fused_gdn_decode_post_conv_mtp is declared inside the VLLM_ENABLE_FUSED_KDA_DECODE block, but the op is registered under VLLM_ENABLE_FUSED_GDN_DECODE and the build defines the two macros from separate architecture lists. KDA decode is built for 9.0a/10.0f/12.0f while GDN decode is also built for 8.0/8.6/8.9, so a build restricted to SM80/86 Ampere or SM89 Ada enables the GDN macro without the KDA one and torch_bindings.cpp then references an undeclared function. Move the declaration into its own VLLM_ENABLE_FUSED_GDN_DECODE block so each declaration is gated by the macro that gates its registration. Signed-off-by: lesj0610 <lesj0610@godoiksan.org>
fec9b46 to
f8028c3
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe ARM64 CUDA 13.0 build and release paths now use newer pinned PyTorch builder images. The libtorch stable operator declarations now separate fused GDN decode from the fused KDA decode compile guard. ChangesARM64 CUDA 13.0 build images
Fused decode compile guards
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change corrects declaration visibility for GDN-only builds while keeping KDA excluded where unsupported; the affected configuration builds and registers the expected operation, so no actionable merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Purpose
fused_gdn_decode_post_conv_mtpis declared inside theVLLM_ENABLE_FUSED_KDA_DECODEblock incsrc/libtorch_stable/ops.h, but the op is defined and registered underVLLM_ENABLE_FUSED_GDN_DECODEincsrc/libtorch_stable/torch_bindings.cpp. The build derives those two macros from different architecture lists: KDA decode is compiled for9.0a;10.0f;12.0f, while GDN decode is also compiled for8.0;8.6;8.9. A build restricted to those extra architectures — SM80/86 Ampere or SM89 Ada — therefore definesVLLM_ENABLE_FUSED_GDN_DECODEwithoutVLLM_ENABLE_FUSED_KDA_DECODE, andtorch_bindings.cppreferences a function that was preprocessed away.This PR moves the declaration into its own
VLLM_ENABLE_FUSED_GDN_DECODEblock so each declaration is gated by the macro that gates its registration. No functional change on architectures where both macros are defined.AI assistance: Claude Opus 5 was used during investigation and PR preparation; the submitter reviewed the changes.
Changes
VLLM_ENABLE_FUSED_KDA_DECODEblock afterfused_kda_decodeand open aVLLM_ENABLE_FUSED_GDN_DECODEblock forfused_gdn_decode_post_conv_mtpincsrc/libtorch_stable/ops.h.Test Plan
Preprocessor visibility check for every enabled-macro combination the architecture lists can produce (KDA+GDN, GDN only, KDA only; the neither-macro case is not exercised because neither op is registered there).
ops.his preprocessed standalone, with its#includelines stripped since the declarations are what matter, before and after the change:Guard/registration cross-check:
End-to-end build in the GDN-only configuration, followed by a symbol check of the produced extension:
Test Result
Preprocessor visibility, before the change:
fused_gdn_decode_post_conv_mtpdeclaredfused_kda_decodedeclaredAfter the change:
fused_gdn_decode_post_conv_mtpdeclaredfused_kda_decodedeclaredThe "GDN only" row is the failing configuration:
torch_bindings.cppcompilesops.def("fused_gdn_decode_post_conv_mtp(...)")andops.impl("fused_gdn_decode_post_conv_mtp", TORCH_BOX(&fused_gdn_decode_post_conv_mtp))under#ifdef VLLM_ENABLE_FUSED_GDN_DECODE(csrc/libtorch_stable/torch_bindings.cpp:534,:803-804), so the missing declaration is a compile error rather than a silently dropped op.Guard/registration cross-check:
CMakeLists.txt:FUSED_KDA_DECODE_ARCHSintersects9.0a;10.0f;12.0f,FUSED_GDN_DECODE_ARCHSintersects8.0;8.6;8.9;9.0a;10.0f;12.0f; each list independently addsVLLM_ENABLE_FUSED_KDA_DECODE=1/VLLM_ENABLE_FUSED_GDN_DECODE=1to_C_stable_libtorch.git diff --check: passed.End-to-end build with
TORCH_CUDA_ARCH_LIST=8.0, which is exactly the "GDN only" configuration:cmake --build --preset release --target installcompleted;_C_stable_libtorch.abi3.sowas produced.cuobjdump --list-elf: 59 ELF images, allsm_80, no other architecture.fused_gdn_decode_post_conv_mtp(Tensor mixed_qkv, Tensor a, Tensor b, Tensor A_log, Tensor dt_bias, Tensor state_indices, Tensor cu_seqlens, Tensor num_accepted_tokens, Tensor! state, Tensor output_gate, Tensor norm_weight, Tensor! out, float scale, float norm_eps=1e-5) -> ().fused_kda_decodeis absent from the binary, as expected for an architecture outsideFUSED_KDA_DECODE_ARCHS.So the configuration that the mismatch breaks now builds and registers the op, with KDA correctly excluded.
Essential Elements of an Effective PR Description Checklist
Summary by CodeRabbit