Conversation
Acquire device-written descriptors in the consuming warp when switching experts. Reuse acquires across consecutive tiles to avoid per-tile overhead. Add an eager/graph regression test that changes tensor addresses while reusing a cached plan. Signed-off-by: Jiangfei Duan <jiangfeid@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe grouped-GEMM weight-gradient kernel now fences TMA descriptor acquisition when processing new experts and reuses descriptors across consecutive tiles. A parametrized CUDA test validates eager execution and repeated CUDA graph replays across supported configurations. ChangesWgrad TMA descriptor reuse
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable merge-blocking risk remains from the reviewed changes. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the bug, fix, rationale, compatibility impact, and regression scope. However, it does not select an affected area, does not include a related issue entry, and does not provide exact testing commands and results. The required label checklist item is also unchecked.
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
@cudnn-ci-bot run oss |
|
🏁 Pipeline finished SHA: 23 passed, 1 failed, 6 manual
|
YangXu1990uiuc
left a comment
There was a problem hiding this comment.
Codex bot review · model: gpt-6-astra
Reviewed ba50412200dd against base d34a6909c9d1.
The fix makes sense, and I found no additional issue. The acquire is in each warp that actually issues the relevant TMA operation; reusing it across consecutive tiles is valid because the descriptor is immutable during this GEMM launch. The discrete-output path also acquires for empty experts. Existing approval is already in place, so this is a follow-up comment rather than a duplicate approval.
Validation: Independent SM100 run at ba50412: all 8 new eager/graph regression cases passed, with no skips. Running the same test against merge-base 7ac5354 reproduced a graph-replay failure in the first dense/2D/overwrite case: 65536/65536 elements differed, max absolute error 1792. The baseline run deliberately stopped at that first failure. Checked descriptor construction, consuming warps and persistent expert transitions against the PTX tensor-map proxy ordering rule: ordinary inter-kernel ordering does not substitute for the required tensor-map acquire. See the PTX memory model. Correctness-gated head/base/base/head timing on the same 68-SM SM100 GPU, CUDA 13.2 and CuTe DSL 4.8: CPU steady-state wrapper enqueue stayed about 14 us for E=4/features=256/tokens=512 and 15 us for E=8/features=1024/tokens=8192. GPU graph replay was 10.23-10.25 us before versus 10.24 us after for dense/2D, and 27.13-27.20 us before versus 28.14-28.18 us after for discrete/ragged (about 3.7%). This small measured cost is acceptable for the correctness fix. Both versions passed exact output checks, poisoned-output replay and changed-input replay in these stable-address timing cases. Reused maintainer-triggered pipeline 68473550 after verifying its mirror records the full reviewed GitHub SHA. OSS SM80/SM90/SM100/SM107 jobs, builds and C++ sanitizer jobs passed. The remaining api_index failure is the missing SdpaFwdDslSm100.template_params entry, outside this two-file change; no duplicate CI run was requested.
Limitations: Local hardware coverage is SM100; SM103 was not independently exercised. The performance check covers two FP8 block-scaled shapes and the complete captured wrapper path, not a broad tuning sweep or an isolated GEMM-only measurement. The overall CI pipeline is not fully green because of the separate api_index failure.
Comment only: this PR already has an active approval; preserving existing approvals.
Before submitting
pre-commit runand committed any formatting changes.AGENTS.mdfor each directory this PR touches (see root AGENTS.md § Reviewing a PR) and my changes comply, or I explain the exception below.cat-*, one or morearea:*/op:*, and oneorig-*(see label list).Affected area
Summary
Fix incorrect SM100 grouped-wgrad results during CUDA graph replay when consecutive calls reuse a tensor-map workspace with different input, scale, or output addresses. The same sequence passes in eager mode on the tested GB300 stack.
Acquire device-written descriptors in the consuming warp when switching experts. Reuse acquires across consecutive tiles to avoid per-tile overhead. Add an eager/graph regression test that changes tensor addresses while reusing a cached plan.
Why
Rewriting a descriptor in global memory does not by itself invalidate stale descriptor state used by TMA. The descriptor-update kernel writes through the generic memory proxy, but the GEMM lacks the required tensor-map acquire, so it can keep using the previous mapping.
Eager launch-time cache invalidation appears to mask this omission on the tested stack. Inserting an ordinary launch of an empty kernel makes the failing sequence correct; inserting the same kernel as a graph node does not. This isolates a launch-path effect, although we have not directly observed the driver's invalidation commands. Graph execution must not rely on that incidental invalidation between nodes.
Add
cpasync.fence_tma_desc_acquire()before TMA operations to explicitly make the updated descriptors visible. Acquire once per expert transition rather than every tile to limit overhead.API and compatibility impact
Only the kernel fix and regression test are included. No public API, workspace-size, or kernel-launch changes.
And slightly perf regression due to additional acquire.
Testing
Summary by CodeRabbit
Bug Fixes
Tests