Fix to #652: SM100 Grouped GEMM: read the TMEM accumulator before releasing the overlapping stage - #654
Conversation
…ing stage The SM100 dSReLU/SReLU/quant grouped-GEMM epilogues call acc_pipeline.consumer_release() -- publishing the overlapping accumulator stage as empty -- before issuing the TMEM->register cute.copy of a subtile that still lies inside the overlap region, so the MMA producer waiting on that mbarrier may overwrite accumulator columns the epilogue has not read yet. cute.arch.fence_view_async_tmem_load() cannot cover that load: both of its candidate lowerings order only prior operations (PTX ISA 9.3 sections 9.7.17.8.5 and 9.7.17.11.1), and the load has not been issued when the fence executes. Section 9.7.17.6.4.4 specifies tcgen05.ld -> tcgen05.wait::ld -> fence -> mbarrier.arrive for exactly this producer/consumer pair. Eight sibling kernels in the same package already order it that way; wgrad is the same idiom with the same pipeline object. Moves the TMEM read above the fence/release in the three SM100 kernels. Pure statement reorder: the release condition is unchanged, and the loop body is the same multiset of statements before and after. Fixes NVIDIA#652
|
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 (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThe three SM100 grouped GEMM epilogues now read each TMEM accumulator subtile before releasing the overlapping accumulator stage and advancing pipeline state. ChangesGrouped GEMM epilogue synchronization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change reorders the accumulator read before stage release in three grouped-GEMM kernels. No actionable merge-blocking risk remains; the unexecuted SReLU and quant paths should receive normal owner follow-up. Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Rubin kernel has the same pattern but unfortunately we don't have a sm107-compatible device to verify the fix. |
|
Thanks @GarlGuo for the PR. |
|
@saltyminty @Anerudhan can we merge the PR if it looks good? Megatron (the upstream) is affected by this bug and we need to use Megatron for training soon. |
…the overlapping stage (#673) Apply the same fix as #654 to the Rubin (SM107) kernel moe_blockscaled_grouped_gemm_quant_rubin.py, which has the identical pattern: the overlapping accumulator stage was released before the TMEM->register copy of that stage's subtile, allowing the MMA warp to overwrite the accumulator while the epilogue was still reading it. Move the t2r copy ahead of the early-release block so the accumulator is read before consumer_release. The other Rubin kernels (glu, glu_hadamard_quant, dglu) already perform the copy before the release, and wgrad_rubin has overlapping_accum disabled, so only this file is affected. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ing stage (NVIDIA#654) The SM100 dSReLU/SReLU/quant grouped-GEMM epilogues call acc_pipeline.consumer_release() -- publishing the overlapping accumulator stage as empty -- before issuing the TMEM->register cute.copy of a subtile that still lies inside the overlap region, so the MMA producer waiting on that mbarrier may overwrite accumulator columns the epilogue has not read yet. cute.arch.fence_view_async_tmem_load() cannot cover that load: both of its candidate lowerings order only prior operations (PTX ISA 9.3 sections 9.7.17.8.5 and 9.7.17.11.1), and the load has not been issued when the fence executes. Section 9.7.17.6.4.4 specifies tcgen05.ld -> tcgen05.wait::ld -> fence -> mbarrier.arrive for exactly this producer/consumer pair. Eight sibling kernels in the same package already order it that way; wgrad is the same idiom with the same pipeline object. Moves the TMEM read above the fence/release in the three SM100 kernels. Pure statement reorder: the release condition is unchanged, and the loop body is the same multiset of statements before and after. Fixes NVIDIA#652 Co-authored-by: Wentao Guo <wg0420@princeton.edu>
Before submitting
pre-commit runand committed any formatting changes.cat-*, one or moremod-*, and oneorig-*.Affected area
FE OSS kernels or CuTeDSL
Summary
Moves the TMEM→register
cute.copyabove the fence/release block in the epilogue of threeSM100 grouped-GEMM kernels —
dsrelu,srelu, andquant.Related issues
Fixes #652
Not tested:
sreluandquantcarry the identicalsource pattern but were not executed.
quant_rubinkernel has the same defect and is deliberately not touched as no sm_107 hardware was available to validate it on.Summary by CodeRabbit