Skip to content

fix: st.bulk support for size operand with 32-bit length is introduce… - #737

Open
cxxlink wants to merge 1 commit into
deepseek-ai:mainfrom
cxxlink:main
Open

fix: st.bulk support for size operand with 32-bit length is introduce…#737
cxxlink wants to merge 1 commit into
deepseek-ai:mainfrom
cxxlink:main

Conversation

@cxxlink

@cxxlink cxxlink commented Aug 25, 2026

Copy link
Copy Markdown

Problem

Using CUDA 12.9 and ARCH sm_100, compile failed.

ptxas /tmp/tmpxft_0038d88e_00000000-6_a.ptx, line 27; error   : Arguments mismatch for instruction 'st.bulk'
ptxas fatal   : Ptx assembly aborted due to errors

Root cause

image

https://docs.nvidia.com/cuda/parallel-thread-execution/index.html?highlight=st%2520bulk%2520weak%2520shared%25203A%25203Acta#data-movement-and-conversion-instructions-st-bulk

image

https://docs.nvidia.com/cuda/parallel-thread-execution/index.html#release-notes

Fix

Use CUDA_VERSION to distinguish which type to use.

        asm volatile("st.bulk.weak.shared::cta [%0], %1, 0;\n" ::
                     "r"(static_cast<uint32_t>(__cvta_generic_to_shared(smem_ptr))),
#if defined(CUDART_VERSION) and CUDART_VERSION >= 13000
                     "r"(kNumBytes)
#else
                     "l"(static_cast<uint64_t>(kNumBytes))
#endif
                     : "memory");

Validation

Compile pass.

Comment thread deep_ep/include/deep_ep/common/ptx.cuh Outdated
EP_STATIC_ASSERT(kNumBytes % 8 == 0, "`st.bulk` requires size to be a multiple of 8");
#if defined(__CUDA_ARCH__) and (__CUDA_ARCH__ >= 1000)
if (elect_one_sync()) {
#if defined(__cccl_ptx_isa) and (__cccl_ptx_isa >= 900)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: 使用 __cccl_ptx_isa 判断隐式依赖 CCCL 头文件已被包含;若某编译单元未引入 CCCL,会一律走 64 位分支。虽然回退安全,但建议改用 __CUDACC_VER_MAJOR__/__CUDACC_VER_MINOR__ 直接判断工具链版本,减少对 CCCL 宏的隐式依赖。

🤖 v5

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment thread deep_ep/include/deep_ep/common/ptx.cuh Outdated
"r"(kNumBytes)
: "memory");
#else
asm volatile("st.bulk.weak.shared::cta [%0], %1, 0;\n" ::

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: 两个分支的内联汇编字符串完全相同,仅操作数约束不同("r" vs "l")。可将 size 操作数抽出以减少重复,非必需。

🤖 v5

@ds-review-bot

Copy link
Copy Markdown
Collaborator

🤖 ds-review-bot Code Review

v6

该变更按 PTX ISA 版本选择 32 位或 64 位 size 操作数,兼容旧版 PTX,同时保留新版行为;未发现会破坏现有构建或运行逻辑的问题。

v5

该 MR 修复了 st_bulkst.bulk.weak.shared::cta 指令 size 操作数的 PTX ISA 兼容性问题:32 位 size 操作数是 PTX ISA 9.0 才引入的,原代码无条件使用 32 位寄存器约束 "r"(kNumBytes),在较旧工具链(PTX ISA < 9.0)下会生成非法 PTX。修复通过 __cccl_ptx_isa &gt;= 900 条件编译区分:新 ISA 使用 32 位操作数,旧 ISA 回退到 64 位 "l"(uint64_t)。宏未定义时安全回退到 64 位分支(对所有 ISA 版本均合法)。修改正确且符合 NVIDIA PTX 文档,变更范围小、风险低,建议合并。

v4p

该 MR 针对 st.bulk 指令 size 操作数的 PTX ISA 兼容性问题:PTX ISA 9.0 起才支持 32 位 size 操作数,因此在 __cccl_ptx_isa >= 900 时继续使用 32 位寄存器("r"),否则回退为 64 位寄存器("l")。整体看,变更范围小、条件编译逻辑清晰,符合现有 inline PTX 的写法,未发现正确性或结构性问题。

Files reviewed: 1
Issues found: 🔵 2 suggestion
Inline comments posted: 2

@cxxlink
cxxlink requested a review from ds-review-bot August 25, 2026 08:45
@cxxlink
cxxlink marked this pull request as draft August 26, 2026 02:01
@cxxlink
cxxlink marked this pull request as ready for review August 26, 2026 02:01
Comment thread deep_ep/include/deep_ep/common/ptx.cuh Outdated
EP_STATIC_ASSERT(kNumBytes % 8 == 0, "`st.bulk` requires size to be a multiple of 8");
#if defined(__CUDA_ARCH__) and (__CUDA_ARCH__ >= 1000)
if (elect_one_sync()) {
#if defined(CUDA_VERSION) and (CUDA_VERSION >= 13000)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 critical: #if defined(CUDA_VERSION) and (CUDA_VERSION >= 13000):CUDA_VERSION 定义在 cuda.h(driver API 头)中,本文件只包含 cuda_bf16.h,nvcc 编译 .cu 时隐式包含的是 cuda_runtime.h,并不引入 cuda.h。因此在未显式包含 cuda.h 的编译单元中该宏未定义,会一律走 64 位分支;而 CUDA 13 的 ptxas 要求 32 位 size 操作数,64 位分支会再次触发 'Arguments mismatch for instruction st.bulk' 编译错误。建议改用 nvcc 始终定义的 __CUDACC_VER_MAJOR__ >= 13(或 CUDART_VERSION)判断。上一轮评审已提出同类建议,作者回复 Done 但当前代码仍使用 CUDA_VERSION,问题未真正解决。

🤖 v5

Comment thread deep_ep/include/deep_ep/common/ptx.cuh Outdated
"r"(kNumBytes)
: "memory");
#else
asm volatile("st.bulk.weak.shared::cta [%0], %1, 0;\n" ::

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: 两个分支的内联汇编字符串完全相同,仅 size 操作数约束不同("r" vs "l"(uint64_t))。可仅对操作数做条件编译以消除重复,非必需(沿用上一轮建议)。

🤖 v5

@ds-review-bot

Copy link
Copy Markdown
Collaborator

🤖 ds-review-bot Code Review

v6

该变更为 PTX ISA 9.0 之前的工具链改用合法的 64 位 size 操作数,能够修复 CUDA 12.9 下的编译失败,未发现破坏现有行为的问题。

v5

该 MR 修复 st.bulk 指令 size 操作数的 PTX ISA 兼容性问题:PTX ISA 9.0(CUDA 13.0)将 size 操作数改为 32 位,旧工具链需用 64 位操作数。修复思路正确,但最新版本改用的 CUDA_VERSION 宏来自 cuda.h,ptx.cuh 并未包含该头文件(仅包含 cuda_bf16.h),nvcc 也不保证隐式定义它。若宏未定义,将无条件回退到 64 位分支——而在 CUDA 13 的 ptxas 下 64 位 size 操作数恰恰会再次触发本 MR 要修复的 'Arguments mismatch for instruction st.bulk' 错误,即回退分支在新工具链上并不安全。上一轮评审已建议改用编译器内置的 CUDACC_VER_MAJOR,作者回复 Done 但代码未落实。

v4p

本 MR 修复了 st_bulk 中 st.bulk.weak.shared::cta 指令 size 操作数的 PTX ISA 兼容性问题:32 位 size 操作数在 PTX ISA 9.0(CUDA 13.0)才引入,旧工具链(如 CUDA 12.9)编译 sm_100 时使用 "r"(kNumBytes) 会触发 ptxas 参数不匹配。变更通过 CUDA_VERSION >= 13000 条件编译,在旧工具链回退到 64 位 "l" 操作数,新工具链保持 32 位操作数。整体看变更范围小、版本判断正确;该头文件通过 compiled.cuh 引入 cuda_runtime.h,宏定义来源有保障,未发现正确性或结构性问题。

Files reviewed: 1
Issues found: 🔴 1 critical | 🔵 1 suggestion
Inline comments posted: 2

@cxxlink
cxxlink requested a review from ds-review-bot August 26, 2026 11:12
@cxxlink
cxxlink marked this pull request as draft August 27, 2026 05:38
@cxxlink
cxxlink marked this pull request as ready for review August 27, 2026 05:39
Comment thread deep_ep/include/deep_ep/common/ptx.cuh Outdated
: "memory");
#else
asm volatile("st.bulk.weak.shared::cta [%0], %1, 0;\n" ::
"r"(static_cast<uint32_t>(__cvta_generic_to_shared(smem_ptr))),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 suggestion: 两个分支的内联汇编字符串完全相同,仅 size 操作数约束不同("r"(kNumBytes) vs "l"(static_cast<uint64_t>(kNumBytes)))。可仅对操作数部分做条件编译以消除重复;也可考虑用编译器内置的 __CUDACC_VER_MAJOR__ >= 13 判断,直接反映生成 PTX 的 nvcc 版本而非运行时头版本。均非必需,沿用前两轮建议,不阻塞合并。

🤖 v5

@ds-review-bot

Copy link
Copy Markdown
Collaborator

🤖 ds-review-bot Code Review

v6

变更使用已明确引入的 CUDART_VERSION 区分 PTX ISA 9.0 前后的 size 操作数宽度,可修复 CUDA 12.9 编译失败,未发现破坏现有行为的问题。

v5

本 MR 修复 st.bulk 指令 size 操作数的 PTX ISA 兼容性问题:PTX ISA 9.0(CUDA 13.0)将 st.bulk 的 size 操作数从 64 位改为 32 位,旧工具链(如 CUDA 12.9 + sm_100)使用 32 位 "r"(kNumBytes) 会触发 ptxas 'Arguments mismatch for instruction st.bulk' 错误。最新版本改用 #if defined(CUDART_VERSION) and CUDART_VERSION &gt;= 13000 区分分支,并显式添加 #include &lt;cuda_runtime.h&gt;,保证该宏在所有包含本头文件的编译单元中均有定义。前一轮关于 CUDA_VERSION 宏可能未定义、导致 CUDA 13 下回退到非法 64 位分支的 critical 问题已解决。变更范围小、逻辑正确,可以合并。

v4p

该 MR 修复 st.bulk 内联 PTX 在 CUDA 12.9 / sm_100 下因 size 操作数位数与 PTX ISA 版本不匹配导致的 ptxas 编译失败:PTX ISA 9.0(CUDA 13.0)起 size 操作数改为 32 位,旧工具链需使用 64 位操作数。当前实现显式包含 cuda_runtime.h 并通过 CUDART_VERSION >= 13000 区分两种约束,与仓库中 compiled.cuh 的既有判断模式一致;之前讨论中关于 CUDA_VERSION 宏未定义的问题已修复,未发现新的正确性或结构性问题。

Files reviewed: 1
Issues found: 🔵 1 suggestion
Inline comments posted: 1

@cxxlink
cxxlink requested a review from ds-review-bot August 27, 2026 05:44
@cxxlink
cxxlink marked this pull request as draft August 27, 2026 08:38
@cxxlink
cxxlink marked this pull request as ready for review August 27, 2026 08:39
@ds-review-bot

Copy link
Copy Markdown
Collaborator

🤖 ds-review-bot Code Review

v6

变更按 nvcc 主版本正确选择 st.bulk 的操作数宽度,兼容 CUDA 12.9 与 CUDA 13+,未发现破坏现有行为的问题。

v5

该 MR 修复 st.bulk 指令 size 操作数的 PTX ISA 兼容性问题:PTX ISA 9.0(CUDA 13.0)将 size 操作数从 64 位改为 32 位,旧工具链(如 CUDA 12.9 + sm_100)使用 32 位 "r"(kNumBytes) 会触发 ptxas 'Arguments mismatch for instruction st.bulk' 错误。当前版本已采纳历史评审的全部建议:使用 nvcc 内置的 CUDACC_VER_MAJOR >= 13 判断工具链版本(不再依赖 cuda.h/cuda_runtime.h 的 CUDA_VERSION/CUDART_VERSION 宏),且条件编译仅作用于 size 操作数部分,汇编字符串不重复。此前提出的 critical 问题(宏未定义导致 CUDA 13 下回退到非法 64 位分支)已彻底解决。变更范围小、逻辑正确,未发现新问题,可以合并。

v4p

本 MR 修复 st.bulk 指令 size 操作数位数在 PTX ISA 9.0 前后的兼容性问题:CUDA 13+(PTX ISA 9.0)使用 32 位操作数,旧工具链回退到 64 位操作数,从而解决 CUDA 12.9 + sm_100 下 ptxas 'Arguments mismatch for instruction st.bulk' 的编译失败。改动范围小,使用 CUDACC_VER_MAJOR 直接判断生成 PTX 的 nvcc 版本,并已按历史讨论将条件编译收敛到操作数部分;未发现正确性、结构或性能问题。

Files reviewed: 1
Issues found: None — LGTM! ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants