[jhinpan]

Adds .claude/skills/review-pr, the FlyDSL PR reviewer. It follows the review-pr skills in ROCm/ATOM and ROCm/aiter, so the three repos review in one format. It replaces the engine in #1106 and #1157, which are closed, and absorbs the parts of that work that do not depend on its architecture.

/review-pr <N> in Claude Code, or run_one.sh <N> headless, ends in one card per PR: a one-sentence summary, an advisory verdict, what CI at the head actually ran, the CI benchmark rows for the changed kernels, at most five findings, each tagged [verified] or [inferred] and ending in an action, and any corrections from the independent refuter. The skill never posts to GitHub.

What comes from where

	ATOM	aiter	#1106 / #1157	here
reviewers	one	one, plus an independent refuter	nine angle finders, a verifier per candidate, a challenger per CONFIRMED, a sweep	one, plus an independent refuter
rules	ATOM PRs, all in one file	aiter PRs, derived per diff	delegated to the existing skills	77 rules from FlyDSL PRs, derived per diff (median 11 over the last 150 merged PRs); deep procedures defer to the existing skills
evidence	prose	fetch.sh artifacts	pinned checkout, two scanners	head and merge target pinned by SHA, CI checks, CI benchmark rows, diff scan, buffer-bound and test-coverage evidence
proof each step ran	—	7 gates	schema-checked artifacts	7 gates
bar for [verified]	—	an evidence chain	arithmetic run, chain walked to an observable	same as #1106
headless	—	workflow on a self-hosted runner	resumable runner, local watcher	run_one.sh; agents optionally in a Docker sandbox with the checkout read-only and no GitHub token; usage.txt per agent

Evidence specific to FlyDSL:

• scripts/compare_benchmark.py returns 0, so the CI benchmark step cannot fail and nothing reads it. fetch.sh pulls the ratio= rows out of the PR's own test-job logs, and triage.py benchcov names the changed kernels no benchmark reaches.
• make_buffer_tensor defaults to max_size=True, i.e. no byte bound; triage.py bounds lists added descriptors without one.
• PR CI runs gfx942, gfx950 and one Navi runner, so a path only gfx1250 reaches, or a test behind a module-level skip, is called out instead of read as green.

Relation to earlier work

The rule set reuses the review-history mining in #1083 (closed), checked against today's tree (for example SMEM_CAPACITY_MAP has moved to python/flydsl/runtime/device.py) and extended with later cases (PRs 1130, 1136, 1153, 1177, 1199, 1200, 1220).

From #1106 and #1157, this PR absorbed: the head pinned by SHA and the diff computed from the pinned commits; "run the arithmetic, do not narrate it" and "walk the chain to an observable" as the bar for [verified]; a refuter that does not kill findings for depending on realistic GPU state; the sweep's list of where a first pass misses; correctness outranking convention under the cap; deferring deep procedures to the owning skills; the legacy-spelling scan patterns; deleted barriers, NaN guards, FileCheck lines and tests as removed-guard leads; and a per-agent usage record. Their publisher, untrusted-container profile, watcher and unreachable-test scanner stay on their branches for when reviews are posted automatically.

Sample runs

Both ran run_one.sh with FLYDSL_REVIEW_SANDBOX_IMAGE=rocm/sgl-dev:v0.5.19-rocm720-mi35x-20260915 on an MI355X node (no GPU used). All seven gates were green and the refuter killed no finding. Nothing was posted to either PR.

Replay of PR 950 (merged Aug 3; the change whose bf16 cost PR 1009 later bisected), at this PR's head. Worker 783 s, refuter 193 s; $5.50 + $1.46 as priced by the CLI.

FlyDSL PR 950 (replay) — [Perf]optimize the flash attention fp8 performance for gfx950

Rewrites the gfx950 fp8 dual-wave flash-attention main loop into a barrier-fenced phase-pipelined schedule selected by new FA_* environment variables, and puts the XCD block remap behind a new default-off trait that only the fp8 builder turns on, for sequences of at least 64 q blocks.

Review (advisory): 🔴 HIGH RISK
CI (head d618179): NONE — no checks are reported at this head, so no GPU test job (mi325-1 / mi35x-1 / navi-2) ran on this code; nothing is classified PR, baseline or infra because nothing ran.
Perf (CI benchmark, advisory): NOT MEASURED — no completed test (linux-flydsl-*) job exists at this head, the PR body carries no numbers, and the only benchmark that reaches either changed file is tests/kernels/test_flash_attn_fwd.py.

🔴 [verified] kernels/attention/flash_attn_utils.py:927 turns the previously unconditional XCD block remap into if const_expr(traits.XCD_SWIZZLE and not traits.SPLITK and not traits.CAUSAL and traits.NUM_HEADS_Q % 8 == 0), and XCD_SWIZZLE defaults to False on both traits classes (:1482, :1766). Only kernels/attention/flash_attn_fp8_gfx950.py ever passes xcd_swizzle=True; the bf16/f16 builder at kernels/attention/flash_attn_gfx950.py:102 — which flash_attn_interface._build_dense and _build_varlen call for dense, varlen and paged attention — still calls _make_dualwave_swp_traits without it, and _init_dualwave_thread_mapping is shared by both contexts (:3290 bf16, :4377 fp8). Impact: every non-causal, non-split-K bf16/f16 config with num_heads % 8 == 0 is compiled with the plain grid mapping instead of the XCD-local one. The remap is a bijection and the code comment says output is bit-identical, so no correctness test can see it and no benchmark ran here — the loss is silent and will only show up as lower long-context throughput. The same gate also disables the remap for fp8 below 64 q blocks (seq_len <= 16128 at BLOCK_M=256), which was previously on at every length. Author must thread xcd_swizzle through the bf16 builder (or keep the old unconditional behaviour there) and post base-vs-head numbers for one long-context bf16 shape showing the mapping change is intended.

⚠️ [verified] kernels/attention/flash_attn_fp8_gfx950.py:182-184 reads FA_PP, FA_PP_PRIO and FA_TPV from os.environ at trace time inside the @flyc.kernel body and feeds them to const_expr, so they pick between three materially different loop schedules. None of them is in DualwaveSwpFp8Traits.cache_tag (kernels/attention/flash_attn_utils.py:1772-1800) and none is in _CACHE_INVALIDATING_ENV_VARS (python/flydsl/compiler/jit_function.py:128, five FLYDSL_* names), and the kernel source text is identical for every value, so the key cannot separate them. Impact: two runs sharing FLYDSL_RUNTIME_CACHE_DIR that differ only in FA_PP compute the same key, and the second is served the first's binary — a stale artifact and a mis-attributed measurement, by construction silent. The defaults also leave code dead in every shipped build: with FA_PP=1 the if const_expr(not PP) arm never compiles, and that arm no longer carries the sched_group_barrier pairs this PR deleted, so FA_PP=0 is a third never-benchmarked schedule rather than the pre-PR fallback. Author must either hard-code the chosen schedule and delete the dead arms, or move the knobs into python/flydsl/utils/env.py with FLYDSL_* names and add them to the fp8 cache_tag.

⚠️ [inferred] kernels/attention/flash_attn_fp8_gfx950.py:252 opens a wave-group barrier shift with _stagger_extra_barrier_if_one(ctx.stagger_i32) (waves 4-7 take one extra s_barrier, since stagger_i32 is wave_id // 4), gated only on FA_PP and never closed. The bf16 sibling does both halves of this and gates both on the trait: kernels/attention/flash_attn_gfx950.py:244 opens, :680 closes with _stagger_extra_barrier_if_zero before the store, each under if const_expr(traits.DUALWAVE_SWP_ENABLE_STAGGER). Impact: dualwave_swp_enable_stagger=False is now inert on the fp8 path while still sitting in the fp8 cache_tag, so the public kwarg silently does nothing; and the two wave halves stay one barrier phase apart through the epilogue. I could not reach a wrong value — the 6-deep K/V ring with a j+4/j+5 lookahead leaves one buffer of slack and store_final_o uses permlane32_swap rather than LDS — so this is a question, not a defect. Reviewer should ask why the fp8 path needs no closing barrier when the bf16 path does, and whether the stagger should honour traits.DUALWAVE_SWP_ENABLE_STAGGER.

⚠️ [verified] The PR title claims a gfx950 fp8 speedup and the body is empty: no hardware, baseline, optimized number, units, command, ROCm version or cache state, for a diff that replaces the main loop of kernels/attention/flash_attn_fp8_gfx950.py and changes two softmax helpers plus the grid mapping in kernels/attention/flash_attn_utils.py. No CI check ran at this head and scripts/compare_benchmark.py returns 0 unconditionally, so even a green run would not have caught a regression. Impact: nothing on either side of this merge can tell a reviewer whether the new schedule is faster, or by how much, on any shape — exactly the gap that let two earlier gfx950 attention regressions sit unnoticed. Author must attach base-vs-head timings for at least one short and one long-context shape (the XCD threshold changes behaviour across seq_len 16128 vs 16129), taken with FLYDSL_RUNTIME_ENABLE_CACHE=0, and state GPU, ROCm version and sample count.

📝 [verified] Two changes in kernels/attention/flash_attn_utils.py reach past the fp8 kernel the title names. _score_pair_sum (:413) is rewritten from a 32-step scalar fadd chain to lo + hi followed by Vec.reduce("add", ...), and its call site at :3726 is inside DualwaveSoftmaxHelper — the bf16/f16 dual-wave kernel, whose emitted reduction therefore changes too (whether the f32 result changes depends on how fastmath was already reassociating it). And MIN_Q_BLOCKS_XCD_SWIZZLE = 64 (:36) arrives with no derivation: at BLOCK_M=256 it moves the XCD crossover to seq_len = 16129, and nothing says on which shapes that crossover was measured. Reviewer should ask whether the bf16 path was re-benchmarked after the helper rewrite, and for the measurement behind the 64-block threshold.

Independent reader's corrections (not yet applied to the findings above):

• Finding 1: flash_attn_interface._build_dense (:88) builds the generic module (flash_attn_generic.build_flash_attn_func_module), not the dual-wave bf16 one; the affected callers are _build_dense_dualwave (:122/:140), _build_varlen (:186/:204) and the paged builders at :273 and :322.
• Finding 3: the claim that dualwave_swp_enable_stagger=False is "now inert" on the fp8 path is wrong — grep for stagger in merge-target/kernels/attention/flash_attn_fp8_gfx950.py finds no stagger call at all, and DUALWAVE_SWP_ENABLE_STAGGER on the fp8 traits is referenced only by the dataclass field (:1702) and the cache_tag (:1769). The kwarg was already inert there; what this PR adds is an unconditional open that ignores the trait rather than honouring it.

Caveats: the replay ran offline from git, with no PR description and no CI, so the empty-description finding is an artifact of the replay. The first finding's rule was written from PR 1009's fix, so it shows the mechanics, not unbiased recall. The second finding appears in no rule's examples and is still on main: kernels/attention/flash_attn_fp8_gfx950.py:183-185 reads FA_PP, FA_PP_PRIO and FA_TPV at trace time, and none of them reaches the JIT cache key.

#1227 (open, RMSNorm tuning for gfx1250), run with this PR's first commit. About 15 min end to end.

FlyDSL PR #1227 — [Kernel][Perf] Tune RMSNorm for gfx1250: BF16 hardware conversion, early loads, small-N kernel choice

Adds five gfx1250-only tuning switches to the three RMSNorm forward builders — hardware bf16 conversion, gamma loaded in pass 1 behind a scheduling barrier, bounds checks dropped on fully-interior tiles, a single-barrier block reduce, and a reciprocal mean — plus a new rule for choosing between the one-row and multi-row small-N kernels.

Review (advisory): ⚠️ NEEDS WORK
CI (head bb146e1): all green — test (linux-flydsl-mi325-1 / gfx942), test (linux-flydsl-mi35x-1 / gfx950), test (linux-flydsl-navi-2), plus Python/C++ style, CodeQL and docs; Multi-GPU SKIPPED (no multi-gpu label). No red checks, so nothing to classify as PR / baseline / infra. Every behavioral change in this diff is gated on arch == "gfx1250", which none of the three GPU runners provides.
Perf (CI benchmark, advisory): rmsnorm 32768x8192 bf16 ratio 0.995–1.000x vs main@1bd7ebd5, 3 rows, one run each on a shared runner — this confirms the "no change off gfx1250" claim and nothing else. NOT MEASURED for the changed paths: no CI benchmark row reaches gfx1250, fused-add RMSNorm, or any N ≤ 2048 shape where the new kernel-choice rule decides.

⚠️ [inferred] kernels/norm/rmsnorm_kernel.py:137 and :875 turn on gamma preloading for every gfx1250 row width, but preload_weight_vec (kernels/norm/rmsnorm_common.py:95) only halves the cached vector for 16-bit gamma — with FP32 gamma it falls through to load_weight_vec, caching 8 VGPRs per tile instead of 4. BF16 activations with FP32 weights are a supported combination (rmsnorm_common.py:151) and the PR's own new test uses it. At N = 32768 on gfx1250, default_block_threads returns 256 (its 512 branch is gfx95-only), so NUM_VEC_ITERS = 16 and gamma_local holds 128 VGPRs rather than the 64 behind the quoted 252-VGPR budget; the fused kernel hoists x, residual and gamma together, so it is worse. Impact: spills to scratch or the s_set_vgpr_msb bank switching this PR exists to avoid — a silent slowdown against main on exactly the wide rows it claims 2.4x on, with no wrong values. The body's "F32 rows" do not cover this: f32 activations set elem_bits = 32 and never enter the vector path at all. Reviewer should ask for rmsnorm(bf16 x, fp32 weight) at N = 16384 and 32768 on gfx1250, base vs head, with the VGPR counts — or for PRELOAD_GAMMA / HOIST_LOADS to be conditioned on the cached gamma width.

⚠️ [inferred] The MULTI_ROW_MIN_M = N rule at kernels/norm/rmsnorm_kernel.py:625 is still contested and the thread is unresolved: zhiding512 measured 4096×1536 and 4096×2048 at 0.88x and 0.78x against main on a second gfx1250 part, jhinpan's bb146e1 answered the ISA cause and asked for a re-run, and no re-run has been posted (the review state is DISMISSED, not approved). The body itself says the 131-row sweep behind the rule "was not re-tuned" after the two changes that followed it. Impact: on a higher-latency gfx1250 part, M between N and 8192 at N = 1024–2048 silently takes the slower kernel — a regression against main on shapes main handles well. Reviewer should ask zhiding512 to re-measure the two rows and the M = N / M = N+1 pairs at bb146e1 before merge, and hold the fallback (one workgroup per row for N > 768 at M ≤ 8192) ready.

⚠️ [verified] tests/kernels/test_rmsnorm.py:1926 skips the new NaN test unless has_hw_cvt_pk_bf16_f32(get_rocm_arch()), which admits gfx95x and gfx1250. On gfx950 — the only PR runner it admits — the predicate this PR replaced already returned True on main, so the test passes identically on the parent commit there; on gfx1250, where the author reports all 8 cases failing on main, no CI job exists. Impact: the three green GPU ticks are a skip or an unchanged pass for every line this PR adds, and the regression test pins nothing that CI can check. It still has value as a forward guard on gfx950. Author must attach the gfx1250 run showing the 8 cases failing on the parent and passing at bb146e1, since no runner can reproduce it.

📝 [inferred] kernels/norm/rmsnorm_kernel.py:1005 hoists x, residual and gamma in the fused kernel but, unlike its two siblings at :410 and :706, emits no fx.rocdl.sched_barrier(0). The PR's own comment at :99 says hoisting alone is not enough because the scheduler sinks the loads back below the first wait; x and residual are anchored by tile 0's add, but gamma is not consumed until the pass-2 loop at :1055, which is exactly that case. Impact: possible speed left on the table in the fused path, not a correctness issue. Reviewer should ask whether the barrier was tried in the fused kernel and measured worse, or simply not tried.

📝 [verified] The new capability predicate at kernels/norm/rmsnorm_common.py:118 is not adopted by kernels/norm/layernorm_kernel.py, which imports the same module yet keeps three hand-copied (arch == "gfx950") or str(arch).startswith("gfx95") expressions (lines 102, 506, 1074) and its own _to_elem_vec at line 68. The PR body declares this out of scope. Impact: after merge, gfx1250 RMSNorm preserves FP32-weight NaN payloads in bf16 output while gfx1250 LayerNorm still flushes payloads ≥ 0x7FFF8000 to ±0 — two norms on one chip with different NaN semantics. Reviewer should ask for a follow-up issue, and whether the predicate belongs next to get_warp_size in python/flydsl/runtime/device.py rather than in a kernel helper.

Independent reader's corrections (not yet applied to the findings above):

• Finding 1: the PR body does acknowledge the mechanism ("FP32 gamma takes two 128-bit loads, as before") -- what is missing is any measurement, not awareness; and the body's two "F32 rows" are M=64 at N=7168/1536, which could not show this regardless of whether they are f32 activations or f32 weights.
• Finding 2: the affected band is 768 < N <= 2048, slightly wider than the card's "N = 1024-2048".
• Finding 4: the card's supporting citation ":99" is misfiled -- that comment is at rmsnorm_kernel.py:139-141 ("the scheduler sinks the gamma loads ... below the first wait") and in Motivation item 2 of the PR body; the siblings are at :421 and :706, not :410 and :706. The claim itself is unaffected, and it is correctly scoped as perf-only.

This run predates the later commits, so it shows the refuter's corrections as they are now listed, but not the stricter prompts.

Trying it

/review-pr 1227                                   # in Claude Code, from a FlyDSL checkout with gh logged in
.claude/skills/review-pr/run_one.sh 1227          # headless; set FLYDSL_REVIEW_SANDBOX_IMAGE=<image> for the Docker sandbox

Not included

• A bot that posts reviews. It needs a self-hosted runner or a watcher, a bot identity and a model endpoint; the publisher and watcher on the [Skills] Add flydsl-code-review skill and workflow #1106 / feat(review): harden review engine for unattended runs #1157 branches are the starting point.
• CI for the skill's own tests. python3 -m pytest .claude/skills/review-pr/test_triage.py runs locally only: .claude/ is outside scripts/run_tests.sh, and a selftest workflow would trigger the GPU matrix on every skill edit.

[jhinpan]

cc @zhiding512 This is ready for review and test.

[zhiding512]

LGTM.