[Skills] Add PR review and performance-regression validation skills

[zhiding512]

Summary

Adds two skills under .claude/skills/:

• review-pr — a static advisory reviewer whose rules are derived from this repository's review history, not ported from another project.
• validate-kernel-pr — a deterministic executor that runs base and head and produces a machine-checkable validation_report.json, including a performance stage.

Draft: this is tooling that applies to the whole repo, so I would like agreement on the rule set and on the perf gate's decision rule before it is treated as usable.

Motivation

FlyDSL has no automated gate on performance. scripts/compare_benchmark.py prints ratios and ends with:

    print("\nBenchmark comparison report completed.")
    return 0

return 0, unconditionally. A regression cannot turn a PR red, and nothing else in the pipeline checks speed — the numbers land in a job log nobody is required to read.

#1009 is what that costs: a default-off trait placed in front of the bf16 XCD remap shipped a silent regression that lived in main for two weeks. Restoring the mapping recovered +17.4% at S=180,180 and +19.4% at S=239,580, with bit-identical output.

A second motivation is that the failure modes here are mostly quiet. Of ~30 defects reconstructed from merged bugfix PRs, 11 returned silently wrong numbers, 5 were silent perf regressions, 2 were tests or benchmarks that lied, and only 6 crashed. A checklist for this repo has to be weighted toward what fails without saying anything.

Changes

review-pr

Rules were mined from 796 human inline review comments (Copilot's 1134 filtered out) and the diffs of ~40 merged bugfix PRs. Every rule cites a real PR. Categories reflect what actually breaks here:

Category	Representative evidence
JIT cache-key direction	over-keying breaks AOT reuse (#624); a runtime value leaking into the key (#556); a new trait missing from cache_tag (#1020)
Gate and flag threading	validation bypassed by a new branch (#1020, #844); a default-off trait orphaning sibling builders (#1009); a surviving False and (#650)
Architecture capability	wave size derived from is_rdna_arch — gfx1250 is wave32 and not matched by RDNA prefixes (#1024); prefix too wide (#544, #943)
Index width / ABI	fx.Int64 widening (#1064); flattening before launch, where _LayoutPlan packs shapes as int32 (#1020)
Data-movement math	truncating division in a copy decomposition (#1064, #1007); atom width vs register vector width (#650, #564); a mask guarding fewer dims than the address fuses (#969)
Numerics	fx.* wrapper missing @dsl_math_wrap_result, costing 11.1–11.9% silently (#1035); ballot body assuming a per-lane predicate (#1033)
Layout algebra	adaptor value/attr mismatch (#1052); getValue() without isStatic() (#926); slice not moving the base (#707)
Tests and benchmarks	a suite that cannot fail (#1056, #1033, #899); a parser keeping the last match (#654)

Two things are deliberate:

• A "sibling diff" step of its own. In [Bugfix][Kernel] Fix NaN from the lazy rescale on a wide score range #1033, [Fix] conv3d fp8: guard padded M rows in the epilogue store #969, [Bugfix] softmax: enable vectorized fast path (fix fastmath attr + f32 128b copy width) (#627) #650, [Fix] Let fx.maxnumf inherit the ambient fastmath #1035 and [Fix] Restore the gfx950 XCD mapping for bf16 dense attention #1009 the correct code already existed in a sibling path and the bug was the asymmetry. That is the highest-yield move in this repo, so it is a step, not a footnote.
• A hard evidence threshold. No high-severity finding may be emitted without naming the concrete shape/dtype/arch/value that triggers it; otherwise it is downgraded or dropped. This is the main defence against plausible-sounding false positives landing on your PR.

scan_flydsl_diff.py turns the mechanical checks into a deterministic candidate list, so a review works a fixed set of sites rather than whatever the model happened to notice.

validate-kernel-pr

Stages: merge_sim, gpu_claim, runtime_compat, test_policy, correctness, perf, diff_scan.

The perf stage is the point. Design is A/B/A interleaved with the noise floor measured during the run:

• Sandwiching head between two base runs cancels monotonic drift. Running all of base then all of head charges every clock ramp to the patch.
• The two base runs are a genuine A/A control; their disagreement is this machine's noise floor for that row, measured now rather than guessed.

A fixed threshold cannot work here, and run_benchmark.sh already says why:

the widest tier at M=64 fills 0.25 workgroups per CU on a 256-CU gfx950 and swings 24% run to run

A 5% gate would flag that row on roughly every other run.

Two FlyDSL-specific hazards are handled explicitly:

• Warm-cache measurement. When the patch touches lib/, include/, python/flydsl/, kernels/common/ or tools/, the JIT key does not move with the change and a warm cache serves the previous kernel — a perfectly reproducible measurement of the wrong binary. The executor forces FLYDSL_RUNTIME_ENABLE_CACHE=0 and records why. Base and head also get separate cache directories, so base cannot serve head.
• Labelled metric parsing. fix(run_benchmark): report base norm bandwidth, not the last variant (layernorm 1.69 → 5.6 TB/s) #654 kept the last regex match and reported layernorm at 1.69 TB/s for months against a real 5.6 — and the current-vs-main gate could not catch it, because main was mislabelled identically.

Testing

python3 -m pytest .claude/skills/validate-kernel-pr/tests/test_validator.py -q   # 23 passed
python3 scripts/check_repo.py                                                    # all checks pass
bash scripts/check_python_style.sh --base origin/main --head HEAD                # clean

Every stage has been observed failing on a seeded defect and passing on a matched control. That pairing is the point: a stage only ever observed passing is decoration, not a check.

Property	Seeded defect	Negative control
Regression detected	head 20% / 6% slower	identical code, 12 seeds, stays green
Noise not mistaken for signal	—	24% run-to-run noise, 12 seeds, never blocks
Drift cancelled	real regression under 3%/round drift	drift alone is not a regression
Unresolvable case reported honestly	5% loss under 24% noise	reported unchanged, not a coin flip
Direction correct (#848 shipped inverted columns)	throughput and latency both ways	—
Row labels preserved (#654)	two rows, same metric	last row must not overwrite the first
Incomplete run cannot claim PASS	perf skipped	all stages passed

Live A/B/A run through real subprocesses, seeded −20% on one row:

perf stage: fail | 1 row(s) regressed beyond the measured noise floor
verdict   : BLOCK
  gemm|4096,4096,4096|bf16      unchanged      +0.0%  noise_floor= 3.0%  control_dev= 0.0%
  layernorm|32768,8192|bf16     regression    -19.5%  noise_floor= 3.3%  control_dev= 2.4%

Also run end-to-end against a real PR (#1077) on this machine, where it correctly returned INCONCLUSIVE rather than blaming the author: the local prebuilt runtime is 0.3.1 while the checkout is 0.3.2, so every kernel import dies on cannot import name 'get_warp_size' — a symbol #1024 added. runtime_compat classifies that as an environment fact and skips correctness and perf instead of filing a red result.

The agent-docs checker also caught three drifted paths in my own skill text (including a LayoutUtils.cpp that does not exist); fixed in the second commit.

Performance

No runtime code is touched — this adds tooling only, so there is no kernel performance impact.

Dependencies

• No new third-party dependencies. Uses gh, git, amd-smi and the stdlib.

Breaking Changes

None. Additive; no existing skill, script or workflow is modified.

Known limitations

Stated in the skill rather than implied:

• No --pr N orchestration. The caller creates the worktree and names --tests / --bench-cmd; an irrelevant target can still produce PASS. The report names both so a reviewer can reject that evidence.
• No LLVM-pin-change detection. If a patch moves the pin, both sides need rebuilding (cf. [CI] Build the benchmark baseline wheel against the base commit's LLVM #1071) or the comparison is meaningless.
• No gfx1250 coverage — no runner in the CI matrix reports gfx1250, and this executor cannot cover an architecture it has no device for.

Review asks

1. Are the rule severities calibrated the way you would want, particularly the cache-key and gate-threading families?
2. Is the perf decision rule (A/A control as the noise floor, with a 3% floor under it) the right shape, and are the default 5 rounds enough for the benchmark set you care about?
3. Should the perf stage eventually run in CI, or stay a local/manual gate?

Made with Cursor