Skip to content

docs(csrc): condense verbose comments (comments-only, no code change)#4061

Draft
carlushuang wants to merge 11 commits into
ROCm:mainfrom
carlushuang:carhuang/condense_comment
Draft

docs(csrc): condense verbose comments (comments-only, no code change)#4061
carlushuang wants to merge 11 commits into
ROCm:mainfrom
carlushuang:carhuang/condense_comment

Conversation

@carlushuang

Copy link
Copy Markdown
Collaborator

What

Condense over-verbose comments across the C/C++/HIP sources under csrc/ and gradlib/. This is a comments-only change: no code, no behavior, no build changes.

Motivation: a lot of kernels have grown 10+ line prose essays explaining a single constexpr or wrapping code in decorative ==== TITLE ==== banners. The canonical example is the MXFP4_MOE_SORT_COALESCED_LDS_WORK_MAX gate, which carried a 12-line rationale essay for one integer. These essays bury the load-bearing facts and make the kernels harder to read.

Scope

86 files changed, net ~1,650 fewer lines. All changes are comment text only.

Condensed: rationale/"why" essays, decorative banner boxes, comments that merely restate the adjacent code, and duplicated explanations.

Preserved verbatim: SPDX/license headers, ASCII memory-layout and bit-packing diagrams, math derivations, correctness invariants, TODO/FIXME/BUG notes, and all code (including commented-out dead code).

Every technical fact in a condensed comment is still recoverable from the shorter version.

Example

Before (csrc/kernels/quant_kernels.cu):

// Perf gate threshold for the coalesced LDS-staged store path in
// mxfp4_moe_sort_kernel (see below). The LDS staging + double __syncthreads
// cost scales with the LDS footprint per row (scaleN_pad bytes) times the
// number of grid tiles (num_blocks). On MI355X/gfx950 uncoalesced byte stores
// are already cheap, so the coalesced path is a net win for small/mid
// (scaleN_pad x num_blocks) but loses once that product is large (empirically
// the only regressor is dim=7168 @ M=16384: 224*4352=974,848; the largest
// winner is dim=7168 @ M=8192: 224*2304=516,096). The threshold sits between
// them with ~35% margin. This is a PURE PERFORMANCE knob: both the coalesced
// and the scatter paths are byte-exact, so the value only affects speed, never
// correctness.
constexpr long MXFP4_MOE_SORT_COALESCED_LDS_WORK_MAX = 700000;

After:

// Perf gate for the coalesced LDS-staged store path in mxfp4_moe_sort_kernel
// (below): cost scales with scaleN_pad (LDS footprint/row) x num_blocks (grid
// tiles). Threshold picked empirically between the largest known win (516,096)
// and the smallest regression (974,848) on MI355X/gfx950. Perf-only: both the
// coalesced and scatter paths are byte-exact.
constexpr long MXFP4_MOE_SORT_COALESCED_LDS_WORK_MAX = 700000;

Verification

For every changed file, comments were stripped from both the base and the edited version and the remaining code token streams compared: byte-identical in all 86 files. No code was altered.

Largest reductions: custom_kernels.cu (-201), fused_qk_norm_rope_cache_quant.cu (-180), topk_plain_kernels.cu (-92), and the opus_gemm gfx950 pipelines.

@carlushuang carlushuang requested a review from a team July 2, 2026 11:24
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:triton-300x Run an additional Triton test job on MI300X in PRs; main branch always runs both MI35X and MI300X
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: GPT-OSS-120B, DeepSeek-R1-0528, Kimi-K2.5
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 4061 --add-label <label>

@carlushuang carlushuang marked this pull request as draft July 2, 2026 13:06
@zufayu zufayu requested a review from junhaha666 July 3, 2026 02:16
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