review(mk-gemm v2): #308 봇 스레드 5건(현재 장치 SM 수, 트레이스 도구 워밍업 감지·메인 스트림·빈 세그먼트) + 원장 30차 §6 노출 프로브·규칙 검증·스탬프 - #313
Merged
Merged
Conversation
…ce; trace tool detects the warm-up step, picks the main stream over the analysed steps, skips empty segments, counts streams in one pass; ledger 30차 §6 -- overlap probe, rule check, unit stamps - glm53_megakernel.cu: g_mk_sms from cudaGetDevice's ordinal, not 0 (codex thread on set_kernel_attrs). - tools/trace_launch_positions.py: the warm-up interval is skipped only when detected (span > 3x the median of the others, >= 3 intervals) so a decode-only or two-anchor trace keeps its steps; the main stream is the busiest stream over the analysed steps, not the whole trace; per-stream kernel counts come from the same pass; an empty segment is skipped instead of crashing max() (copilot + codex threads). - MEASUREMENTS.md 30차 §6 (srv2 01:37-01:39): the MoE-overlap probe -- shared-expert pair exposure 47.3-47.7 us/layer on the persistent lane vs 18.6-28.6 on v2 (-0.8 to -1.2 ms/step); the rule check -- v2 x2 faster than v1 x2 on every m <= 16 shape, m = 32 level, exact gates PASS; the unit stamps (96-unit shapes: skew 0.1, first record 7.5, tail 5-9 us). Every offline gate for EXP-21 is green; the bracket is the operator's. - README / runbook: the numbers above. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6362538c-e966-4d60-8e34-e8742fc6a116) |
There was a problem hiding this comment.
🟡 Changes recommended
The SM-count fix is incomplete as persistent-grid sizing still queries device ordinal 0 (risking incorrect grid/barrier behavior on non-zero devices), and a small trace-tooling cleanup is also needed for consistency.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR follows up on PR #308 review items by fixing device-index correctness for SM-count discovery in the megakernel, refining offline trace analysis (warm-up detection, main-stream selection scoped to analysed steps, empty-segment handling), and documenting/recording the latest “ledger 30차 §6” measurements and gate outcomes.
Changes:
- Megakernel: read
g_mk_smsusingcudaGetDevice()+cudaDeviceGetAttribute(..., dev)instead of hardcoding device ordinal 0 (mirrored into build copies). - Trace tooling: detect-and-skip warm-up only when clearly present; choose main stream by busiest kernel time within analysed steps; skip empty step segments safely.
- Docs/tests: update logic contract checks and record new measurement results and gate status in docs.
File summaries
| File | Description |
|---|---|
| tools/trace_launch_positions.py | Adds warm-up detection and scopes “main stream” selection to analysed decode steps; handles empty segments. |
| tests/test_logic.py | Updates contract assertions to match the new cudaGetDevice()-based SM query. |
| overlay/modules/glm53_megakernel/glm53_megakernel.cu | Switches g_mk_sms SM-count query from device 0 to the current CUDA device. |
| build/glm53/glm53_megakernel.cu | Mirrors the same current-device SM-count query change into the build copy. |
| build/dsv4/glm53_megakernel.cu | Mirrors the same current-device SM-count query change into the build copy. |
| RUNBOOK_KERNEL_CAMPAIGN2.md | Updates offline gate status and references the latest §6 measurements. |
| overlay/modules/glm53_megakernel/README.md | Updates v2 vs persistent lane narrative with §6 overlap probe results. |
| MEASUREMENTS.md | Adds §6 measurement round (overlap probe, rule validation, stamps) and expected serving impact. |
Review details
Suppressed comments (1)
tools/trace_launch_positions.py:94
- The module docstring still says "main = the stream carrying the most kernels", but the implementation (and this comment) now defines main stream by busiest kernel time over analysed steps. That mismatch can confuse readers and users comparing outputs across tools; update the docstring to match the time-based definition.
# the main stream is the one carrying the model's forward: the stream
# with the most kernel TIME over the analysed steps (the routed MoE
# kernels alone are ~half a step). A kernel-count heuristic picks the
# glue stream, the step anchor's stream is not it either -- the 09-01
# stock trace has the prep kernels and the shared-expert pair on 17 and
- Files reviewed: 8/8 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3089
to
+3092
| int dev = 0; | ||
| MK_CHECK_CUDA(cudaGetDevice(&dev)); | ||
| MK_CHECK_CUDA(cudaDeviceGetAttribute( | ||
| &g_mk_sms, cudaDevAttrMultiProcessorCount, dev)); |
Comment on lines
+3089
to
+3092
| int dev = 0; | ||
| MK_CHECK_CUDA(cudaGetDevice(&dev)); | ||
| MK_CHECK_CUDA(cudaDeviceGetAttribute( | ||
| &g_mk_sms, cudaDevAttrMultiProcessorCount, dev)); |
Comment on lines
+3089
to
+3092
| int dev = 0; | ||
| MK_CHECK_CUDA(cudaGetDevice(&dev)); | ||
| MK_CHECK_CUDA(cudaDeviceGetAttribute( | ||
| &g_mk_sms, cudaDevAttrMultiProcessorCount, dev)); |
| 14초 겹침, KV 예산 줄보다 뒤라 무해로 판단) — 어떤 컨테이너든 있으면 대기. | ||
|
|
||
|
|
||
| ### 6. 측정 3차 — 노출 프로브·규칙 검증·유닛 스탬프 (2026-09-05 01:37~01:39, srv2, chain6 뒤 창, `g2.{overlap2,rule,stamps}.out`) |
Comment on lines
+84
to
+89
| spans = [ev[b - 1]["ts"] + ev[b - 1]["dur"] - ev[a]["ts"] | ||
| for a, b in zip(starts[:-1], starts[1:])] | ||
| first = 1 if (len(spans) >= 3 and spans[0] > 3 * statistics.median(spans[1:])) else 0 | ||
| if first: | ||
| print(f"step 0 skipped as warm-up ({spans[0] / 1e3:.0f} ms vs median {statistics.median(spans[1:]) / 1e3:.0f} ms)") | ||
| step_ranges = list(zip(starts[first:-1], starts[first + 1:])) |
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
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.
무엇
PR #308(머지됨)에 달린 Copilot/Codex 인라인 코멘트 5건의 반영과, 마지막 GPU 창(01:37~01:39)의 측정 3종.
코드
glm53_megakernel.cu:g_mk_sms를cudaGetDevice의 현재 ordinal 로 조회(0 고정 → 현재 장치;mk_probe_device와 같은 방식).tools/trace_launch_positions.py: 워밍업 스텝은 감지될 때만 건너뜀(span > 나머지 중앙값 ×3, 구간 ≥ 3 — 디코드만 있는 트레이스·앵커 2개 트레이스는 스텝 보존), 메인 스트림은 분석 대상 스텝 안에서 커널 시간이 가장 큰 스트림, 스트림 요약의 커널 수는 같은 패스에서, 빈 세그먼트는max()대신 건너뜀. 두 트레이스(무장 09-04: 스텝0 339 ms 감지·main 17, 스톡 09-01: 4,505 ms 감지·main 210)에서 확인.원장 30차 §6 (srv2,
g2.{overlap2,rule,stamps}.out)--gemm2 both, x2): in_proj 75.8/79.9, 4096² 47.6/57.3, k2048 27.6/32.8, n2048 28.7/32.8, 6144 69.1/73.7, k3072 37.9/46.1, 1024 21.0/22.5, k512 12.9/15.4, m16 4096² 50.2/58.9 — m ≤ 16 전 형상 v2 < v1, m=32 ±1 µs 동률. exact PASS(두 레인 × ksr 5종).−1.2 → **−1.3−1.8 ms/스텝(2~3%)**, 트레이스 mk_gemm 합 14.1 →78 ms. 오프라인 게이트 전부 통과 → EXP-21 브래킷(base 기본값 → candVLLM_GLM53_MK_GEMM2=1)은 운영자 승인 대상.검증
cudaGetDevice블록 하나).test_logic메가커널 계약·스냅샷 검사 통과; 전체 스위트는 같은 창에서.🤖 Generated with Claude Code
Note
Low Risk
Changes are a device-index correctness fix, offline trace tooling, and documentation; serving behavior only shifts if the process was on a non-zero GPU with a wrong SM count before.
Overview
Follow-up to PR #308 review items plus ledger 30차 §6 GPU measurements (overlap probe, rule validation, unit stamps).
Megakernel (
glm53_megakernel.cu, mirrored build copies):g_mk_smsis now read withcudaGetDevice+cudaDeviceGetAttribute(..., dev)instead of device ordinal 0, aligningmk_choose_ksr2slot math with the GPU actually used for launches (same pattern asmk_probe_device).test_logic.pycontract checks updated accordingly.tools/trace_launch_positions.py: Step analysis skips a detected warm-up interval (first span > 3× median of later spans, only when ≥3 intervals). Main stream is chosen from kernel time inside analyzed decode steps (avoids long init on another stream winning). Empty step segments are skipped instead of callingmax()on an empty list.Docs:
MEASUREMENTS.mdadds §6 (MoE‖shared-expert exposure 47→29 µs/layer on v2, rule-validated x2 beats v1 for m≤16, stamps, −1.3~−1.8 ms/step expectation).RUNBOOK_KERNEL_CAMPAIGN2.mdand megakernel README mark offline gates passed and refresh v2 vs persistent overlap numbers (m=32 tie, pair-first serving order).Reviewed by Cursor Bugbot for commit 261263a. Bugbot is set up for automated code reviews on this repo. Configure here.