review-pr

name	review-pr
description	AI code review for ATOM PRs. ATOM consumes aiter kernels and integrates with vLLM/SGLang plugins. Reviews check perf claims, aiter cross-repo deps, model coverage, dispatch correctness, and AI-generated code patterns. Invoke with a PR number.
argument-hint	<PR number>

ATOM PR Review

ATOM is a ROCm/AMD GPU kernel optimization layer (MI300X/MI355X) that:

• Consumes aiter ops (attention, MoE, GEMM, norm, quant)
• Integrates with vLLM and SGLang as a plugin/backend
• Provides custom MLA, sparse attention, TBO, and quantization fusion

A change here can break inference for all models using the affected kernel path.

---

Step 1 — Fetch

PR=$1
REPO="ROCm/ATOM"

gh pr view $PR --repo $REPO --json title,body,number,labels,files,author,reviews,comments > /tmp/pr_meta.json
gh pr diff $PR --repo $REPO > /tmp/pr.diff

# Linked issue
ISSUE=$(cat /tmp/pr_meta.json | python3 -c "
import json,re,sys
body = json.load(sys.stdin).get('body','') or ''
m = re.search(r'(?:fix|close|resolve)[s]?[: ]*#(\d+)', body, re.I)
print(m.group(1) if m else '')
")
[ -n "$ISSUE" ] && gh issue view $ISSUE --repo $REPO --json title,body > /tmp/pr_issue.json

# Prior reviewer comments (top-level)
cat /tmp/pr_meta.json | python3 -c "
import json,sys
d = json.load(sys.stdin)
for r in d.get('reviews',[]):
    b = (r.get('body','') or '').strip()
    if b: print(f'[REVIEW {r[\"author\"][\"login\"]}] {b[:200]}')
for c in d.get('comments',[]):
    b = (c.get('body','') or '').strip()
    if b: print(f'[COMMENT {c[\"author\"][\"login\"]}] {b[:200]}')
"

# Inline review comments (line-level — often more specific than top-level)
gh api "repos/$REPO/pulls/$PR/comments" | python3 -c "
import json,sys
comments = json.load(sys.stdin)
for c in comments:
    author = c.get('user',{}).get('login','')
    body = (c.get('body','') or '').strip()
    path = c.get('path','')
    line = c.get('line') or c.get('original_line','')
    if body and 'copilot' not in author.lower() and 'bot' not in author.lower():
        print(f'[INLINE {author}] {path}:{line}')
        print(f'  {body[:250]}')
" 2>/dev/null

Read the diff and PR body before proceeding.

Cross-file verification — before reporting any kernel/dispatch finding. The diff shows changed lines, not the whole story. Grep the entire symbol family (.cu + .cuh + .h, or the whole module), not just files in the diff — sync/fence/atomics or the "other half" of a scatter often live in a header, and dispatch/else-branch completeness must be read in the full function, not the hunk. A "no synchronization" or "missing branch" finding based only on the diff is how false positives happen.

Classify every CI failure before blaming the PR. A red check is not automatically the PR's fault:

• Read the failed step/service. An external docs/readthedocs.com build failing on a .claude/-only or code-only change is unrelated infra (those files do not feed the RTD sphinx docs) — do not treat it as a content failure (ATOM#1549).
• Compare against main: if main fails the same job in the same window, it is baseline/flaky, not a regression introduced here.
• Expired CI logs (HTTP 410 Gone) on old runs mean the failure is months-stale and meaningless against today's main — ask for a rebase + fresh run instead of quoting it.

---

Step 2 — Semantic Understanding (answer before rules)

Q1 — What specifically changed computationally? Not "improves MLA" — which kernel path, what data flow, what formula? Answer:

Q2 — Hardware + model scope: which arch(es), which model family/families? gfx942 / gfx950? DeepSeek V3 / V3-0324 / Kimi K2.5 / GPT-OSS / GLM? TP config? Answer:

Q3 — Does this introduce or modify aiter op usage? New from aiter import X? New kwargs on existing aiter calls? Removed aiter calls? Answer:

Q4 — Performance claim: what is the mechanism? Not "faster" — WHY? (fewer kernel launches, fused allreduce+norm, better tiling?) Answer:

Q5 — Does the description explain WHY or only WHAT? "Enable ar+norm+quant fusion" = surface. "Eliminates 2 intermediate HBM round-trips between allreduce, rmsnorm, and quant by calling the aiter fused kernel" = understanding. Answer:

---

Step 3 — PR Type Classification

• Performance optimization → P1 (benchmark numbers), P5 (setup cost excluded?), P2 (production shapes), P3 (reproducible)
• New aiter op usage / aiter API change → E1 (aiter dep), B5 (param added/removed/renamed propagation), B6 (new kwarg unhandled by all ATOM dispatch branches?), E3 (dead param)
• API signature change (param added / removed / renamed in an ATOM function or base class, default or return type changed) → B5 (propagation to all receivers), E1 (aiter dep if aiter-side), E2 (plugin bridge sync)
• New constexpr / routing flag / new attribute key added → B6 (do ALL dispatch branches handle the new value, or assert on it?), C3 (new arch string literal?)
• Model integration (atom/models/*.py) → A2 (shared backbone coverage), Step 4 risk
• Dispatch logic change → B1 (silent bypass), B2 (phase proxy), B6 (new value unhandled?), A3 (scope too broad)
• cudagraph / capture path → B3 (cudagraph compatibility)
• Plugin / vLLM / SGLang interface → E2 (bridge sync)
• Config / tuning → A2 (other GPU archs), A3 (scope too broad)
• Bug fix → A1 (sibling variant), HK3 (regression test); root cause explained?
• FP8 / quantization → C1 (fnuz by dtype), C2 (dtype hardcoded)
• Weight transform / new weight attr → F1 (double HBM pin)
• Async / multi-stream / weight prep → G1 (stream sync missing), G1b (blocking queue.get without timeout in serving code)
• New if/elif dispatch with variable assignment → D1b (UnboundLocalError on uninitialized path)
• New @compile_ops / torch.library.custom_op, or change to an op's return dtype/arity → D7 (fake/abstract impl exists?), D6 (fake dtype/shape matches real op?)
• Kernel launcher / buffer-offset or index arithmetic (long-context or large-batch path) → D9 (int32 overflow at production scale)
• New aiter / C-extension kernel call → D8 (contiguous check)
• Removes or reverses a zero-init / assert / .contiguous() / documented invariant → D4 (invariant reversal cited?)

---

Step 4 — Backbone File Risk Assessment

What makes an ATOM file "backbone"? Apply these questions to any file in the diff.

Q1 — Tier 1 test: Is this file executed on EVERY forward-pass request,
     regardless of which model is being served?
     (model_runner, engine_core, scheduler, config = YES)
     → YES → Tier 1 (system-critical: every inference is affected)

Q2 — Tier 1 alt: Is this file a base class inherited by >2 production model
     implementations, so a bug here affects all of them even if the PR says
     "model-specific fix"?
     (deepseek_v2.py is base for DSv2/V3/V3-0324/Kimi = YES)
     → YES → Tier 1

Q3 — Tier 2 test: Does this file implement an op (attention, linear, norm, MoE)
     that is shared across >1 model family, where a correctness bug
     silently produces wrong results for all users of that op?
     (attention_mla.py, linear.py, moe.py = YES)
     → YES → Tier 2

Q4 — Tier 3 note: Is this a plugin bridge file (vllm/*.py, sglang/*.py)?
     → Tier 3 by blast radius, but HIGH VISIBILITY — only plugin users
       are affected, but those users see the API break immediately.

Otherwise → Tier 3 (model-specific or kernel-specific).

Key difference from aiter: ATOM has no import atom — Tier 1 is defined by "executes on every request" or "base class for multiple model families", not by import chain.

The table below is the current snapshot; Q1–Q4 classify new files not yet listed.

Backbone files ranked by git commit frequency (2025–2026) and blast radius:

Tier	File	Git commits	Blast radius	Common failure mode
1	atom/model_engine/model_runner.py	158	ALL inference — every forward pass	OOM, cudagraph break, wrong batch assembly
1	atom/config.py	91	All models — config drives dispatch	Wrong model config silently changes kernel path
1	atom/models/deepseek_v2.py	68	DSv2/V3/V3-0324/Kimi base class	Wrong MLA, OOM, accuracy drop for all DSv*
2	atom/model_ops/moe.py	69	All MoE models	Wrong expert routing, double weight pinning
2	atom/model_engine/scheduler.py	68	Request batching for all models	Stall, wrong decode/prefill split
2	atom/model_ops/attention_mla.py	54	All MLA attention paths	Wrong KV, accuracy drop, crash
2	atom/model_ops/attentions/aiter_mla.py	52	aiter MLA dispatch	Wrong kernel, wrong dtype
2	atom/model_ops/linear.py	49	All linear layers (every model)	Wrong GEMM dispatch, wrong quant
2	atom/model_ops/attention_mha.py	46	All MHA models	Wrong attention output
2	atom/model_ops/layernorm.py	32	Norm + quant fusion path	Wrong scale, wrong dtype
2	atom/models/deepseek_v4.py	34	DSv4 / Kimi-K2.5 specific	Wrong sparse MLA, SWA layout break

Tier-1 special rule: When model_runner.py or config.py is touched, ask: does the change interact with cudagraph capture? Any new Python control flow, dynamic tensor allocation, or attribute lookup inside the captured region will silently break cudagraph.

deepseek_v2.py special rule: Base class for DSv2, DSv3, DSv3-0324, and Kimi. A bug here affects all four model families even if the PR says "Kimi-only fix". Check: is the changed method overridden in subclasses? If not, all variants are affected.

Mandatory backbone checks — must be answered before writing the verdict:

For Tier 1 files (model_runner, config, deepseek_v2, scheduler):

• List every function/method changed. Grep for callers: grep -r 'def <name>' atom/models/ atom/model_engine/. If any caller not mentioned in the PR exists, flag it.
• For deepseek_v2.py: run grep -rn 'def <changed_method>' atom/models/ — does any subclass override it differently? If not overridden → all DSv2/V3/V3-0324/Kimi are affected.
• Is there an integration test (full forward pass, not unit test alone) exercising this path after the change? If not → 📝 HK3
• State explicitly: if this change is wrong, what breaks? (crash / silent wrong value / OOM / cudagraph break) and how would it be detected?

For Tier 2 files (moe, attention_mla, linear, aiter_mla, scheduler):

• Which model families use this op/file? List them. Is at least one from each family tested?
• Are production shapes covered? (TP=4, TP=8, decode single-token, prefill ISL≥4096)
• Does the change affect the FP8 path, the BF16 path, or both? If both, are both tested?

AI code red flag — verbatim duplication across backbone files: If the same algorithmic block appears in 2+ backbone files with only variable names changed (same formula, same comments, same structure), ask: was each file's invariant verified independently, or was the fix copy-pasted? See D5.

---

Step 5 — Rule Checklist

Six failure categories — work all six in order. Severity: 🔴 block / ⚠️ should fix / 📝 note.

🔴 gate — before firing any 🔴, write down the concrete input that triggers it. Name the specific shape / scale / dtype / arch / value that makes the finding fire (e.g. "at token_id > 16M with H=32, D=128 the int32 product exceeds 2^31", or "when the new attribute is absent the getattr default silently drops shared-expert slots"). If you cannot state a concrete triggering case, the 🔴 is unproven — downgrade to ⚠️ ("worth checking") or drop it. A 🔴 that reads as a definite blocker but names no demonstrable triggering input is exactly how a false positive lands on a maintainer's PR. This gate applies to every rule below — including those whose own text omits an explicit FP self-check (e.g. D9): the same index expression is safe in a capped/small-batch path and unsafe only at a scale you must actually exhibit.

Category	Core question	Key triggers
A. Coverage gaps	Same bug elsewhere? Shared path other models?	_opt, _prefill_opt, _v2; shared backbone; broad condition
B. Silent bypass	Every input reaches the right branch?	gated-off param; phase proxy (max_q); string alias
C. Hardcoded arch/dtype	Does the constant break on another GPU or config?	bf16 fixed; fp8_e8m0 fixed; gfx942 assumed
D. Uninitialized state	Is the buffer clean before kernel launch?	::empty()+atomic; cudagraph dynamic allocation
E. Cross-repo sync	Does the consumer know?	new aiter symbol; new param nobody passes; plugin bridge
F. Resource duplication	Does the change double HBM silently?	new _preshuffled/_quantized weight alongside original

---

A — Coverage Gaps

"Fixed one path; the same bug lives in a sibling."

A1 — Sibling function/kernel not fixed ⚠️ (🔴 if Tier-1/2 backbone) Fix changes address calc, bounds check, or data layout: scan same file for variants named _opt, _prefill_opt, _decode, _v2. Real example (aiter#3841): strided q_nope fix on decode kernel; _prefill_opt in same file had same bug. → ⚠️ A1: same bug may exist in [variant] — check function family in this file

A2 — Change covers one model/GPU, shared path affects others ⚠️ PR labeled "[MI308]" or "DSv4-only" but touches a backbone file shared with Kimi/DSv3/gfx950:

• Special: deepseek_v2.py is the base class for DSv2, DSv3, DSv3-0324, and Kimi — a "Kimi-only fix" here affects all four.
• If benchmark only shows one GPU arch, ask about the other. Real example (ATOM#1498): "[MI308]" backbone change still affects gfx950 (MI355X). → ⚠️ A2: [change] labeled [scope] but shared backbone [file] also affects [other models/archs]

A3 — Activation condition broader than validated scope ⚠️ New dispatch enables kernel for model family X, tested only on subcase Y. Real example (vLLM#16435): FusedMoE activated for wrong families → follow-up restrict PR needed. → ⚠️ A3: activation condition enables [X] but only [Y] was tested

---

B — Silent Bypass

"The code looks complete but certain inputs silently take the wrong path."

B1 — Dispatch gate with unchecked parameter 🔴