Why aiter#5069's measured -25% kernel win moved serving throughput by 0.0% - #8
Merged
Merged
Conversation
ROCm/aiter#5069 retuned the GLM-5.2 a8w8 and BF16 GEMM configs for gfx950 and reported 49 shapes going 3606.4us -> 2702.1us, zero regressions. A/B'd on one 8x MI355X node with only those four CSVs changed, ten points across both models all landed inside a 0.04-0.35% noise floor. The null result survives falsification twice over: each arm records the sha256 of what it deployed, so the tables really did differ, and GLM-5.2's BF16 lookup misses fell 1256 -> 616, so the change really did engage. The tuning worked. Serving did not move. Section 12 explains why, and none of it is bad luck. `tgemm` has three call sites in SGLang and one is CUDA-only. On a GLM-5.x FP8 checkpoint the live pair is the MoE router and the DSA indexer's unquantised weights_proj -- both tiny. The dense projections take gemm_a8w8_blockscale*, routed experts take fused_moe and tuned_fmoe.csv, and lm_head is a plain torch.matmul. The serving logs agree: the only (N,K) pairs that ever reach the BF16 table are (256, 6144) and (32, 6144). Half the PR cannot be reached at all here -- the non-block-scale gemm_a8w8_bpreshuffle it feeds needs SGLANG_USE_AITER_FP8_PER_TOKEN, and this checkpoint is block quantised, so 99 of its 189 changed gfx950 rows are never read. Two amplifiers sit behind that. The tuner's shape list is a geometric M ladder while serving produces dense arbitrary M, and lookup pads up to nextPow2, so M=6016 runs a kernel tuned for 8192. And GEMM across all backends is 17.3% of real GPU compute -- a profile whose raw form claims the TP all-reduce owns 97.4% until you notice 6 of its 182 calls are ranks waiting, not reducing. The census tool that establishes this is worth more than the finding. Running it on our own published recipe says the gfx950 half of the glm53 table we pin covers M=1 and M=32 only, while chunked prefill drives M to 8192: 1616 distinct shapes miss and 1608 of them fall through to plain torch F.linear. The interesting work is not retuning the rows we have. Also fixes setup_pr.sh, which stopped working on 2026-08-31 when sglang#36507 was rebased. It asserted that the measured commit was still an ancestor of the PR head -- right instinct, but it assumed the branch only moves forward, and reproduction failed at step one. Fetching the exact object pins the tree just as tightly and survives a rebase. Verified against the rebased upstream: the old assertion fails, the new path succeeds.
Reviewer's GuideThis PR turns a surprising null serving result into a reproducible analysis: it proves the tuning was deployed and engaged, shows that most retuned rows are not on the live GLM-5.x ROCm paths or do not match serving shapes, quantifies GEMM’s bounded end-to-end impact, adds a coverage census tool and review checklist, and repairs setup-script reproduction after upstream rebases. Sequence diagram for tuning coverage censussequenceDiagram
participant Workload as Serving workload
participant SGLang
participant AITER as AITER GEMM lookup
participant Table as BF16 tuned table
participant Report as coverage_report.py
Workload->>SGLang: Execute representative load
SGLang->>AITER: Lookup M, N, K
AITER->>Table: Probe exact M
alt tuned row found
Table-->>AITER: Return padded_M and kernel config
else no tuned row
Table-->>AITER: Miss, use torch F.linear
end
AITER-->>SGLang: Run selected solution
SGLang-->>Report: AITER_LOG_TUNED_CONFIG log
Report->>Report: Count hits, misses, tables, and padded_M drift
Flow diagram for validating a tuning claimflowchart TD
Start[Candidate kernel tuning claim] --> Route[Prove the live model route]
Route --> Mechanism[Measure hit and miss changes]
Mechanism --> AB[Run controlled end-to-end A/B]
AB --> Gate{Correctness gate passes?}
Gate -- No --> Reject[Reject performance conclusion]
Gate -- Yes --> Noise[Compare delta with arm noise floor]
Noise --> Conclusion[Report serving win or null result]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="glm53_flash/coverage_report.py" line_range="45-47" />
<code_context>
+
+def base(f): return f.rsplit("/", 1)[-1]
+
+print(f"{len(hits)} distinct shapes hit, {len(misses)} distinct shapes missed\n")
+print("=== distinct shapes per table ===")
+tbl = collections.Counter(base(h["file"]) for h in hits)
</code_context>
<issue_to_address>
**issue (bug_risk):** The report counts every matching log line as a distinct shape, but AITER's cache is per worker process, so a multi-rank server emits the same shape once per rank. The displayed hit/miss totals and percentages are therefore inflated by worker duplication rather than representing distinct shapes.
**Triggers:** When the server log combines lookup output from multiple tensor-parallel worker processes.
**Suggested fix:** Deduplicate records by the lookup key, such as `(M, N, K, table, padded_M, library)`, before computing counts and percentages.
```suggestion
def base(f): return f.rsplit("/", 1)[-1]
def key(h): return (h["M"], h["N"], h["K"], h["file"], h.get("pM"), h["lib"])
hits = list({key(h): h for h in hits}.values())
misses = list({key(h): h for h in misses}.values())
print(f"{len(hits)} distinct shapes hit, {len(misses)} distinct shapes missed\n")
```
</issue_to_address>
### Comment 2
<location path="glm53_flash/setup_pr.sh" line_range="34-39" />
<code_context>
+ || git fetch --no-tags origin 'pull/${SGLANG_PR}/head'
+git cat-file -e '${SGLANG_HEAD}^{commit}' || exit 1
git checkout -q --detach '${SGLANG_HEAD}'
+git merge-base --is-ancestor '${SGLANG_HEAD}' FETCH_HEAD 2>/dev/null \
+ || echo 'note: measured commit is no longer an ancestor of the PR head (rebased upstream); the tree checked out above is still exactly the measured one'
echo '--- after ---'
git log -1 --format='%H %ci %s'
</code_context>
<issue_to_address>
**nitpick (bug_risk):** When fetching `SGLANG_HEAD` succeeds, `FETCH_HEAD` points to that same commit, so `git merge-base --is-ancestor '${SGLANG_HEAD}' FETCH_HEAD` compares a commit with itself and always succeeds. A rebased PR is therefore never reported by the supposedly informational ancestry check on the normal successful path.
**Triggers:** When the exact measured commit can be fetched directly from the remote, including the rebased-PR case described in the change.
**Suggested fix:** Fetch or save the PR head separately and compare `SGLANG_HEAD` against that commit instead of comparing it with the exact-object fetch's `FETCH_HEAD`.
```suggestion
git fetch --no-tags origin 'pull/${SGLANG_PR}/head' 2>/dev/null
PR_HEAD="\$(git rev-parse FETCH_HEAD)"
git fetch --no-tags origin '${SGLANG_HEAD}' 2>/dev/null \
|| git fetch --no-tags origin 'pull/${SGLANG_PR}/head'
git cat-file -e '${SGLANG_HEAD}^{commit}' || exit 1
git checkout -q --detach '${SGLANG_HEAD}'
git merge-base --is-ancestor '${SGLANG_HEAD}' "\$PR_HEAD" 2>/dev/null \
|| echo 'note: measured commit is no longer an ancestor of the PR head (rebased upstream); the tree checked out above is still exactly the measured one'
```
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: glm53_flash/coverage_report.py:47
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+45
to
+47
| def base(f): return f.rsplit("/", 1)[-1] | ||
|
|
||
| print(f"{len(hits)} distinct shapes hit, {len(misses)} distinct shapes missed\n") |
There was a problem hiding this comment.
issue (bug_risk): The report counts every matching log line as a distinct shape, but AITER's cache is per worker process, so a multi-rank server emits the same shape once per rank. The displayed hit/miss totals and percentages are therefore inflated by worker duplication rather than representing distinct shapes.
Triggers: When the server log combines lookup output from multiple tensor-parallel worker processes.
Suggested fix: Deduplicate records by the lookup key, such as (M, N, K, table, padded_M, library), before computing counts and percentages.
Suggested change
| def base(f): return f.rsplit("/", 1)[-1] | |
| print(f"{len(hits)} distinct shapes hit, {len(misses)} distinct shapes missed\n") | |
| def base(f): return f.rsplit("/", 1)[-1] | |
| def key(h): return (h["M"], h["N"], h["K"], h["file"], h.get("pM"), h["lib"]) | |
| hits = list({key(h): h for h in hits}.values()) | |
| misses = list({key(h): h for h in misses}.values()) | |
| print(f"{len(hits)} distinct shapes hit, {len(misses)} distinct shapes missed\n") |
Comment on lines
+34
to
+39
| git fetch --no-tags origin '${SGLANG_HEAD}' 2>/dev/null \ | ||
| || git fetch --no-tags origin 'pull/${SGLANG_PR}/head' | ||
| git cat-file -e '${SGLANG_HEAD}^{commit}' || exit 1 | ||
| git checkout -q --detach '${SGLANG_HEAD}' | ||
| git merge-base --is-ancestor '${SGLANG_HEAD}' FETCH_HEAD 2>/dev/null \ | ||
| || echo 'note: measured commit is no longer an ancestor of the PR head (rebased upstream); the tree checked out above is still exactly the measured one' |
There was a problem hiding this comment.
nitpick (bug_risk): When fetching SGLANG_HEAD succeeds, FETCH_HEAD points to that same commit, so git merge-base --is-ancestor '${SGLANG_HEAD}' FETCH_HEAD compares a commit with itself and always succeeds. A rebased PR is therefore never reported by the supposedly informational ancestry check on the normal successful path.
Triggers: When the exact measured commit can be fetched directly from the remote, including the rebased-PR case described in the change.
Suggested fix: Fetch or save the PR head separately and compare SGLANG_HEAD against that commit instead of comparing it with the exact-object fetch's FETCH_HEAD.
Suggested change
| git fetch --no-tags origin '${SGLANG_HEAD}' 2>/dev/null \ | |
| || git fetch --no-tags origin 'pull/${SGLANG_PR}/head' | |
| git cat-file -e '${SGLANG_HEAD}^{commit}' || exit 1 | |
| git checkout -q --detach '${SGLANG_HEAD}' | |
| git merge-base --is-ancestor '${SGLANG_HEAD}' FETCH_HEAD 2>/dev/null \ | |
| || echo 'note: measured commit is no longer an ancestor of the PR head (rebased upstream); the tree checked out above is still exactly the measured one' | |
| git fetch --no-tags origin 'pull/${SGLANG_PR}/head' 2>/dev/null | |
| PR_HEAD="\$(git rev-parse FETCH_HEAD)" | |
| git fetch --no-tags origin '${SGLANG_HEAD}' 2>/dev/null \ | |
| || git fetch --no-tags origin 'pull/${SGLANG_PR}/head' | |
| git cat-file -e '${SGLANG_HEAD}^{commit}' || exit 1 | |
| git checkout -q --detach '${SGLANG_HEAD}' | |
| git merge-base --is-ancestor '${SGLANG_HEAD}' "\$PR_HEAD" 2>/dev/null \ | |
| || echo 'note: measured commit is no longer an ancestor of the PR head (rebased upstream); the tree checked out above is still exactly the measured one' |
A bare 'exit 1' after the reachability check leaves whoever is reproducing with no idea what happened or what to do. Name the commit, say the branch was rebased and the object collected, and say the right response is to re-measure rather than substitute a different head -- the published numbers are tied to this tree. Also re-verified the fetch itself properly. The earlier check ran in a worktree that already had the object, so cat-file would have passed either way. Repeated it in an empty repo against github.com: fetching the exact SHA succeeds even though the branch no longer descends from it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ROCm/aiter#5069retuned the GLM-5.2 a8w8 and BF16 GEMM configs for gfx950 and reported, from its own measurement, 49 shapes going 3606.4us → 2702.1us (−25.08%), median +22.76% per shape, zero regressions.We A/B'd it on one 8× MI355X node — same image, SGLang worktree, recipe and bench protocol, only the four tuning CSVs changed:
Ten points, all inside a 0.04–0.35% noise floor. Both arms passed the GSM8K gate.
The null result survives falsification twice
b453…vsa361…, bf16c84f…vs01da….N=256, K=6144half the PR added rows for.The tuning worked. Serving did not move. New §12 explains why, and none of it is bad luck.
Almost nothing reads the table that changed
aiter.tuned_gemm.tgemmhas three call sites in SGLang; one is CUDA-only, dead on ROCm. On a GLM-5.x FP8 checkpoint the live pair is the MoE router (N=n_routed_experts) and the DSA indexer's unquantisedweights_proj(N=n_heads) — both tiny. Dense projections takegemm_a8w8_blockscale*; routed experts takefused_moe/tuned_fmoe.csv;lm_headis a plaintorch.matmul. Three different tables, and the recipe's headline GEMMs are in none of the ones this PR touched.The serving logs agree exactly: across both arms the only
(N,K)pairs reaching the BF16 table are(256, 6144)and(32, 6144).Half the PR is unreachable here. The non-block-scale
gemm_a8w8_bpreshufflethata8w8_bpreshuffle_tuned_gemm_glm5.2.csvfeeds needsSGLANG_USE_AITER_FP8_PER_TOKEN; GLM-5.2-FP8 is block quantised (weight_block_size: [128,128]) so it takesgemm_a8w8_blockscale*. 99 of the PR's 189 changed gfx950 rows are never read.Two amplifiers
The tuner optimises M values serving never produces. Shape lists are a geometric ladder (
1 2 4 8 16 24 32 48 64 96 128 …); chunked prefill and continuous batching produce dense arbitrary M (6016 6528 6848 7104 7109 7168 7448 …). Lookup probes exact M → round-up by size band →nextPow2→ give up, soM=6016runs the kernel tuned forM=8192. AITER already ships the fix for the shape list —AITER_TUNE_GEMM=1records what a real workload executes. A ladder of powers of two is what you get when that step is skipped.GEMM is 17.3% of real GPU compute. The profile's raw form claims
cross_device_reduce_2stageowns 97.4% — until you notice 6 of its 182 calls are 98.9% of that, the longest 2.35s against a 282us median. Those are ranks waiting at the collective. With waits removed: TP all-reduce 29.9%, MoE 18.8%, GEMM 17.3%, mHC 10.1%, KDA 6.1%, DSA attention 1.7%. Even a perfect 25% off the whole GEMM bucket is 4.3% end to end.The census tool matters more than the finding
glm53_flash/coverage_report.pyreads anAITER_LOG_TUNED_CONFIG=1server log and answers what an op-level speedup claim cannot: does the model read the table that changed, and at which shapes. Run against our own published recipe:torchF.linearThe gfx950 half of the
glm53_bf16_tuned_gemm.csvthis cookbook pins covers M = 1 and M = 32 only — precisely the two captured decode graph tiers — while prefill drives M to 8192. The interesting work is not retuning the rows we have; the table is nearly empty for this workload.§12.5 turns this into a checklist: route proof, mechanism proof, end-to-end A/B.
Also:
setup_pr.shwas brokenIt stopped working on 2026-08-31 when
sglang#36507was rebased. It asserted the measured commit was still an ancestor of the PR head — right instinct, wrong assumption that a branch only moves forward — so reproduction failed at step one. Now it fetches the exact object, which pins the tree just as tightly and survives a rebase, and downgrades the ancestry check to an informational note.Verified against the actually-rebased upstream: the old assertion fails, the new path succeeds.
Verification
bash verify.sh --renderpasses: offline checks, GLM-5.3 regression tests, DSV4 launch-script parse, published rows byte-identical to the raw records, jsdom render and deep links.Summary by Sourcery
Explain the disconnect between AITER GEMM microbenchmark gains and serving throughput by documenting workload coverage, adding a tuning census tool, and repairing rebased-commit reproduction.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: