[FlyDSL] gfx950 FP8 MQA logits indexer kernel - #4538
Open
vpietila-amd wants to merge 32 commits into
Open
vpietila-amd wants to merge 32 commits into
vpietila-amd wants to merge 32 commits into
Conversation
Bundle every MFMA-shape-derived constant (tile dims, accumulator width, rocdl functor, head-reduce shuffle offsets, accumulator->head layout, fragment size, operand-list builder) into a frozen `MfmaAtom` dataclass, and drive `_build_kernel_mfma_r_w` from it instead of hardcoded 16x16x32 literals. `_MFMA16` reproduces the previous shape and is the default, so this is a pure refactor. This is preparation for additional MFMA shapes; adding one becomes a new MfmaAtom instance rather than a second copy of the builder. No functional change. Verified by cross-compiling for gfx942 (FLYDSL_GPU_ARCH=gfx942 COMPILE_ONLY=1 FLYDSL_DUMP_IR=1) before and after: the final ISA is byte-identical for all 9 variants at H128/D128, and for mfma_r1_w1 / mfma_r2_w4 at H64/D128.
Add the two CDNA4 scaled MFMA shapes and the 32-byte fragment load path they need: * _MFMA16_K128 (16x16x128, requires head_size % 128 == 0) * _MFMA32_K64 (32x32x64, serves head_size 64 and 128) Both carry scaleA/scaleB UE8M0 operands in their encoding, so they use a new _make_operands_scaled_identity that passes a compile-time identity scale; the kernel keeps applying kv_scale in f32 after the MFMA. FlyDSL ships a friendly wrapper for 16x16x128 but not 32x32x64, so the latter gets a thin ODS adapter. Their 32-byte-per-lane fragment exceeds buffer_load's dwordx4 limit, so _load_pack_i32x8 issues two dwordx4 loads and concatenates them. The per-K-step load is factored into _load_frag, which picks the dense 64-bit path or the 32-byte path from the atom. The scaled atoms reject FNUZ, so combining them with the in-kernel FN->FNUZ patch is asserted against. No gfx942 change: MfmaAtom.kname_tag pins _MFMA16's existing symbol name, and the final gfx942 ISA is byte-identical for mfma_r1_w1 / mfma_r2_w4 / mfma_r4_w2 at H128/D128. Validated on gfx950 against the torch reference (op_tests ref_fp8_mqa_logits), max relative error 2.0e-02..2.4e-02 with matching -inf masks, for 32x32x64 at H128/D128, H64/D128, H64/D64 and for 16x16x128 at H128/D128, H64/D128.
Add _build_kernel_mfma_lds_pipe, a second builder for the CDNA4 scaled
atoms that stages KV tiles through LDS instead of re-reading them from
global memory per wave.
* multi-buffer LDS staging (num_buffers) with a configurable
prefetch_depth, filled by cooperative raw_ptr_buffer_load_lds DMA
* the reader-before-writer barrier is elided when num_buffers >
prefetch_depth, since the prefetch target slot then differs from the
slot being read
* optional XOR swizzle of the LDS layout to avoid bank conflicts
* work is partitioned by rows-per-wave here (each wave owns whole rows
across all column tiles) rather than by column tiles as in the
direct-load builder, so all waves share one staged KV tile
_make_out_row_t and _load_pack_i32x8 are hoisted to module scope and
shared by both builders rather than duplicated. buffer_ops is taken from
aiter's vendored copy, matching the rest of the tree.
No gfx942 change: the final ISA is byte-identical for mfma_r1_w1 /
mfma_r2_w4 / mfma_r4_w2 at H128/D128.
Validated on gfx950 against the torch reference with the 3-buffer
swizzled configuration (block_kv=64, num_buffers=3, prefetch_depth=2):
max relative error 2.0e-02..2.4e-02 with matching -inf masks at
H128/D128, H64/D128, H64/D64 (256x256) and H128/D128 (1024x1024).
Register the gfx950 variants and make the default and shape-adaptive
selection arch-dependent.
gfx942 is untouched: the same 9 "mfma_r<RPB>_w<WPB>" tags, the same
mfma_r2_w4 default, and the same _auto_variant heuristic.
gfx950 gains 26 variants tagged
"mfma<MxNxK>_bkv<B>_r<RPB>_w<WPB>[_lds<NUM_BUFFERS>]": 8 direct-load, 13
LDS double-buffered and 5 LDS triple-buffered, over the two scaled atoms
and block_kv in {64, 128, 256}. Its _auto_variant picks the 3-buffer
32x32x64 bkv=64 pipeline, choosing rows-per-wave from the shape (r=2 when
KV streaming pressure is high or the problem is large and square, else
r=1; r=1 always at H>=128).
Supporting details:
* the six near-identical per-atom variant closures collapse into one
parameterised _mk_builder
* _parse_variant reads block_kv and the effective rows-per-block from
either tag scheme, replacing the host-side "mfma_r(\d+)" regex; for
_lds variants a block owns RPB*WPB rows
* _auto_num_splits takes block_kv instead of assuming _BLOCK_KV, which
matters for the bkv=64 and bkv=256 variants
* an unsupported arch now raises a clear NotImplementedError naming the
supported set, rather than failing with an empty registry
Verified: with GPU_ARCHS=gfx942 the registry is exactly main's 9 variants
and default; gfx950 exposes 26; gfx90a raises. On gfx950 the auto
selection reproduces the tuned baseline's choice for every shape checked
(H128 1024x1024, H64 1024x1024, H64 128x32768, H64 8192x8192).
Correctness on gfx950 through the public entry point: 80/80 of
{H64,H128} x {D64,D128} x 10 shapes (1x1 up to 64x8192) x {causal,
misaligned} match the torch reference with correct -inf masks.
Add gfx950 to SUPPORTED_GFX so the existing sweep covers the new kernels, and fix two grading issues it exposes. 1. The reference was graded against the raw bf16 q while the kernels consume q quantized to fp8. kv was already fake-quantized for exactly this reason; q now is too. This charges the kernels only with their own error: measured diffs drop from ~3e-4 to ~1e-6. 2. calc_diff is 1 - 2xy/(x^2+y^2), an aggregate similarity. Over a single finite element it degenerates to (a-b)^2/(a^2+b^2), where one borderline ReLU term -- a dot product near zero flipping sign between fp32 accumulation orders -- moves it by percent. At s_q=s_k=1 with a 1-element window this tripped the 1e-3 bound at 1.2e-3. The FlyDSL and Triton kernels produce the *same* value there and deviate from the fp32 reference identically, so it is not a kernel defect. The aggregate is now asserted only where it is meaningful; checkAllclose still bounds the magnitude in every case. Note the dtype axis degenerates on gfx950: e4m3_type is arch-dependent (FN there, FNUZ on gfx942), so both DTYPE_MAP keys resolve to float8_e4m3fn and the fnuz/fn cases coincide. Only gfx942 has a genuine FN/FNUZ split. Documented in place so the coverage is not overstated. Full sweep on gfx950 (MI355X): 368 cases pass, max diff ~1.2e-06.
* B008: the LDS launcher declared `stream: fx.Stream = fx.Stream(None)`;
drop the default to match the direct-load launcher. The entry point
always passes stream explicitly.
* F841: remove the dead `slot_byte` (only `slot_dword` is used).
* RUF028: drop an inline `# fmt: skip` that is invalid inside an
argument list.
* black formatting.
No functional change: the gfx942 final ISA is still byte-identical to
origin/main for all 9 variants, and the gfx950 sweep still passes 368/368.
* bench_flydsl_vs_triton_fp8_mqa_logits.py -- A/B harness over the
registered FlyDSL variants and the Triton kernel. Enumerates variants
from KERNEL_VARIANTS (--list-variants, --flydsl-variants, "auto"),
times in eager and CUDA-graph modes, and can verify against either the
torch reference or the Triton output (--verification). Emits a
Markdown table with GPU/git provenance.
* _bench_timing.py -- the eager/graph timing primitives it uses
(MeasureConfig, bench_eager, bench_graph, measure). Kept separate
because the CUDA-graph path is not covered by the existing
utils/benchmark_utils.py.
* utils/plot_fp8_mqa_perf.py -- turns the emitted Markdown into grouped
bar charts and a summary table.
Adapted from the development branch: the arch probe now uses aiter's
get_gfx() rather than flydsl.runtime.device.get_rocm_arch, matching the
rest of the tree. Lint fixes on the way in, including a real closure bug
in the plotter (_speedup captured the loop variable triton_row).
Verified on gfx950: shape 15 (bs1 1024x1024 H128 D128) gives flydsl
mfma32x32x64_bkv64_r1_w2_lds3 at 34.2 us / 503.1 TFLOPs, verify PASS.
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
`rocdl.s_waitcnt(N)` called positionally takes a raw gfx9 wait-counter bitfield, not a counter value: vmcnt occupies bits [3:0] and [15:14], expcnt [6:4], lgkmcnt [11:8]. The LDS-pipelined builder passed the plain count `(PREFETCH_DEPTH - 1) * NUM_ASYNC_LOADS`. Below 16 that lands in the vmcnt low nibble and works, though it also asserts lgkmcnt(0)/expcnt(0) -- harmless, since the following gpu.barrier() implies lgkmcnt(0) anyway. At 16 and above it overflows into expcnt and decodes as vmcnt(0): a full drain of every outstanding load. The three `bkv256_*_lds2` variants compute exactly 16 (NUM_ASYNC_LOADS=16, prefetch_depth=2), so their software pipeline never overlapped DMA with compute at all. Switch to the keyword form, which encodes the field correctly, and assert the count fits the 63 the encoding holds -- a deeper prefetch or a wider block_kv would otherwise walk straight back into it. Verified: all three bkv256 variants produce correct output after the fix. Also drops one pre-existing trailing space so the file is black-clean. Co-Authored-By: Claude <noreply@anthropic.com>
vpietila-amd
changed the title
vpietila-amd
marked this pull request as ready for review
August 5, 2026 07:46
vpietila-amd
changed the title
msaffari-amd
reviewed
Aug 5, 2026
msaffari-amd
reviewed
Aug 5, 2026
msaffari-amd
reviewed
Aug 5, 2026
msaffari-amd
reviewed
Aug 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Adds gfx950 support for the FlyDSL FP8 MQA logits indexer, including LDS-staged kernels and fused logits initialization.
Changes:
- Adds gfx950 MFMA variants and automatic selection.
- Fuses
-infoutput initialization into kernels. - Expands and relocates correctness/performance testing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
aiter/ops/flydsl/kernels/mqa_logits/fp8_mqa_logits.py |
Implements gfx950 kernels, dispatch, and fused initialization. |
op_tests/test_flydsl_fp8_mqa_logits.py |
Adds expanded verification and benchmarking. |
op_tests/flydsl_tests/test_flydsl_fp8_mqa_logits.py |
Removes the superseded test location. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1508
to
+1511
| if num_heads <= 32: | ||
| if streaming or large_square: | ||
| return "mfma32x32x64_bkv64_r2_w2_lds3" | ||
| return "mfma32x32x64_bkv64_r2_w4_lds3" |
| + s_k * head_dim # Q + KV (fp8) | ||
| + (s_k + s_q * num_heads) * 4 # scales + weights | ||
| + 2 * s_q * 4 # ks + ke | ||
| + s_q * s_k * 4 # output |
Comment on lines
+980
to
+981
| q_i32 = GTensor(Q, dtype=T.i32, shape=(-1,)) | ||
| kv_i32 = GTensor(KV, dtype=T.i32, shape=(-1,)) |
Comment on lines
1645
to
1646
| if scale_mul != 1.0: | ||
| kv_scales = kv_scales.to(torch.float32) * scale_mul |
amd-sriram
added a commit
to amd-sriram/vllm
that referenced
this pull request
Aug 25, 2026
On gfx950 the sparse-attention indexer's prefill logits run on aiter's Triton/Gluon fp8_mqa_logits. ROCm/aiter#4538 adds a FlyDSL implementation of the same kernel whose launcher takes identical arguments and which is faster on the GLM-5.2 shapes (1.21x geomean over the Gluon kernel in the PR's own numbers, and the kernel is the single largest per-step cost in a 60k-context prefill step). Route mqa_logits_module() at it when VLLM_ROCM_USE_AITER_FLYDSL_MQA_LOGITS is set, so one build can run both arms and any aiter without the PR falls back to the existing path. Off by default while the PR is unmerged. Swapping the module rather than adding a second call site keeps the two kernels sharing one caller, so their arguments cannot drift apart. The lookup is behind mqa_logits_module()'s lru_cache, so the env var is read once per process rather than on every prefill step. The FlyDSL kernel is written against gfx950, so _flydsl_mqa_logits_module() returns None on every other architecture and the Triton/Gluon module is used regardless of the flag. Signed-off-by: Sriram Kumar <sriramkumar.kishorekumar@amd.com>
coderfeli
reviewed
Aug 26, 2026
| def _mk_builder(rpb, wpb): | ||
| return lambda **kw: _build_kernel_mfma_r_w( | ||
| **kw, rows_per_block=rpb, waves_per_block=wpb | ||
| # Kernel-variant registry (arch-dependent). |
Collaborator
There was a problem hiding this comment.
Seperate the kernel and host launch codes into different files. Try not to make it too big.
Collaborator
There was a problem hiding this comment.
also move host codes out of kernel folder like other flydsl kernels.
coderfeli
requested changes
Aug 26, 2026
amd-sriram
added a commit
to amd-sriram/vllm
that referenced
this pull request
Aug 26, 2026
On gfx950 the sparse-attention indexer's prefill logits run on aiter's Triton/Gluon fp8_mqa_logits. ROCm/aiter#4538 adds a FlyDSL implementation of the same kernel whose launcher takes identical arguments and which is faster on the GLM-5.2 shapes (1.21x geomean over the Gluon kernel in the PR's own numbers, and the kernel is the single largest per-step cost in a 60k-context prefill step). Route mqa_logits_module() at it when VLLM_ROCM_USE_AITER_FLYDSL_MQA_LOGITS is set, so one build can run both arms and any aiter without the PR falls back to the existing path. Off by default while the PR is unmerged. Swapping the module rather than adding a second call site keeps the two kernels sharing one caller, so their arguments cannot drift apart. The lookup is behind mqa_logits_module()'s lru_cache, so the env var is read once per process rather than on every prefill step. The FlyDSL kernel is written against gfx950, so _flydsl_mqa_logits_module() returns None on every other architecture and the Triton/Gluon module is used regardless of the flag. Signed-off-by: Sriram Kumar <sriramkumar.kishorekumar@amd.com>
1 task
Contributor
|
hi for the gfx950 perf, do you collect it on mi350 or mi355? |
Contributor
|
FYI, I tested your code on a mi350 machine, it shows that the new gluon in #5048 out-performs |
zhiding512
added a commit
that referenced
this pull request
Aug 31, 2026
A target driven by a `__main__` guard could reach neither correctness_s1_grid nor
execution_receipt, so INCONCLUSIVE was the hard ceiling for every aiter
`op_tests/*.py` -- the repository's standard test shape. Neither skip described a
property of the target:
* the route profiler is sys.setprofile plus a receipt write, and pytest was
only where the hook happened to be installed;
* the grid was injected solely through an environment variable, while these
targets take shapes on the command line. That is a limit of the injector.
Reported as skips, both read as honest abstention while actually hiding a
capability gap -- the failure mode this skill exists to prevent.
The probe is now installed for script targets by run_script_with_probe.py, which
executes the file under runpy with run_name="__main__". It calls the probe's own
hooks rather than re-implementing the profiling, so `producer` stays truthful and
the tested PR still cannot forge a receipt. The probe module is generated whenever
a route is named, not only on the pytest branch; without that the runner had
nothing to import.
--shape-arg names the target's own CLI flag for the grid. The flag is named by the
caller rather than guessed, because a wrong guess appends argv the target silently
ignores. The hook is held to the same standard of proof as the env-var channel: the
flag must appear in an add_argument call, and the existing invalid-grid probe still
has to make the target fail before the stage is credited.
A receipt is now validated whenever a route was named, including when no grid was
configured or the grid channel could not be established. Two branches used to
abandon the receipt along with the grid, discarding evidence already collected. With
no grid it asserts route execution and nothing about shapes, which is all it is then
entitled to claim.
report_schema.json relaxes the PASS constraint `test_selection.runner: const pytest`
to `enum [pytest, script]`, since a script target can now supply both missing
stages. shape_arg is declared but deliberately not required, so reports from earlier
validators stay valid.
Verified on #4538 (gfx950, MI355 OAM), base 78b9440/891788643:
* script target with --shape-arg -s: PASS 9/9, exit 0, review-pr consumable,
receipt naming _auto_variant over the four injected production shapes
* unaccepted --shape-arg flag: correctness_s1_grid skip, receipt still pass
* seeded defect (fused -inf fill dropping [e, seq_len_kv)): BLOCK, exit 1, with
correctness_s1_grid failing independently of the PR's own suite
* tests/test_validator.py 24 passed; PASS/INCONCLUSIVE/BLOCK reports and one
pre-shape_arg report all validate against the schema
zhiding512
added a commit
that referenced
this pull request
Aug 31, 2026
A target driven by a `__main__` guard could reach neither correctness_s1_grid nor
execution_receipt, so INCONCLUSIVE was the hard ceiling for every aiter
`op_tests/*.py` -- the repository's standard test shape. Neither skip described a
property of the target:
* the route profiler is sys.setprofile plus a receipt write, and pytest was
only where the hook happened to be installed;
* the grid was injected solely through an environment variable, while these
targets take shapes on the command line. That is a limit of the injector.
Reported as skips, both read as honest abstention while actually hiding a
capability gap -- the failure mode this skill exists to prevent.
The probe is now installed for script targets by run_script_with_probe.py, which
executes the file under runpy with run_name="__main__". It calls the probe's own
hooks rather than re-implementing the profiling, so `producer` stays truthful and
the tested PR still cannot forge a receipt. The probe module is generated whenever
a route is named, not only on the pytest branch; without that the runner had
nothing to import.
--shape-arg names the target's own CLI flag for the grid. The flag is named by the
caller rather than guessed, because a wrong guess appends argv the target silently
ignores. The hook is held to the same standard of proof as the env-var channel: the
flag must appear in an add_argument call, and the existing invalid-grid probe still
has to make the target fail before the stage is credited.
A receipt is now validated whenever a route was named, including when no grid was
configured or the grid channel could not be established. Two branches used to
abandon the receipt along with the grid, discarding evidence already collected. With
no grid it asserts route execution and nothing about shapes, which is all it is then
entitled to claim.
report_schema.json relaxes the PASS constraint `test_selection.runner: const pytest`
to `enum [pytest, script]`, since a script target can now supply both missing
stages. shape_arg is declared but deliberately not required, so reports from earlier
validators stay valid.
Verified on #4538 (gfx950, MI355 OAM), base 78b9440/891788643:
* script target with --shape-arg -s: PASS 9/9, exit 0, review-pr consumable,
receipt naming _auto_variant over the four injected production shapes
* unaccepted --shape-arg flag: correctness_s1_grid skip, receipt still pass
* seeded defect (fused -inf fill dropping [e, seq_len_kv)): BLOCK, exit 1, with
correctness_s1_grid failing independently of the PR's own suite
* tests/test_validator.py 24 passed; PASS/INCONCLUSIVE/BLOCK reports and one
pre-shape_arg report all validate against the schema
zhiding512
added a commit
that referenced
this pull request
Sep 1, 2026
`Let script targets carry grid and receipt evidence` relaxed the PASS-requires-pytest
assumption in the validator and in report_schema.json, but review-pr re-derives the
verdict independently and still required `selection["runner"] == "pytest"`. A script
target that legitimately reached PASS was therefore rejected by the consumer as
self-contradictory:
validation verdict contradicts its stages/findings: expected INCONCLUSIVE, got PASS
Two of the three places that encode the contract were updated and the third was not,
so the change silently made the evidence unusable at exactly the point it was meant to
be consumed. The coverage-basis check immediately above already handled `script`, which
is what made the omission easy to miss.
Verified against the report from #4538 on gfx950 (script target, --shape-arg
-s, PASS 9/9): rejected before, "validation report accepted for head 2285090..." after.
tests/test_validator.py 25 passed.
zhiding512
added a commit
that referenced
this pull request
Sep 1, 2026
A target driven by a `__main__` guard could reach neither correctness_s1_grid nor
execution_receipt, so INCONCLUSIVE was the hard ceiling for every aiter
`op_tests/*.py` -- the repository's standard test shape. Neither skip described a
property of the target:
* the route profiler is sys.setprofile plus a receipt write, and pytest was
only where the hook happened to be installed;
* the grid was injected solely through an environment variable, while these
targets take shapes on the command line. That is a limit of the injector.
Reported as skips, both read as honest abstention while actually hiding a
capability gap -- the failure mode this skill exists to prevent.
The probe is now installed for script targets by run_script_with_probe.py, which
executes the file under runpy with run_name="__main__". It calls the probe's own
hooks rather than re-implementing the profiling, so `producer` stays truthful and
the tested PR still cannot forge a receipt. The probe module is generated whenever
a route is named, not only on the pytest branch; without that the runner had
nothing to import.
--shape-arg names the target's own CLI flag for the grid. The flag is named by the
caller rather than guessed, because a wrong guess appends argv the target silently
ignores. The hook is held to the same standard of proof as the env-var channel: the
flag must appear in an add_argument call, and the existing invalid-grid probe still
has to make the target fail before the stage is credited.
A receipt is now validated whenever a route was named, including when no grid was
configured or the grid channel could not be established. Two branches used to
abandon the receipt along with the grid, discarding evidence already collected. With
no grid it asserts route execution and nothing about shapes, which is all it is then
entitled to claim.
report_schema.json relaxes the PASS constraint `test_selection.runner: const pytest`
to `enum [pytest, script]`, since a script target can now supply both missing
stages. shape_arg is declared but deliberately not required, so reports from earlier
validators stay valid.
Verified on #4538 (gfx950, MI355 OAM), base 78b9440/891788643:
* script target with --shape-arg -s: PASS 9/9, exit 0, review-pr consumable,
receipt naming _auto_variant over the four injected production shapes
* unaccepted --shape-arg flag: correctness_s1_grid skip, receipt still pass
* seeded defect (fused -inf fill dropping [e, seq_len_kv)): BLOCK, exit 1, with
correctness_s1_grid failing independently of the PR's own suite
* tests/test_validator.py 24 passed; PASS/INCONCLUSIVE/BLOCK reports and one
pre-shape_arg report all validate against the schema
zhiding512
added a commit
that referenced
this pull request
Sep 1, 2026
`Let script targets carry grid and receipt evidence` relaxed the PASS-requires-pytest
assumption in the validator and in report_schema.json, but review-pr re-derives the
verdict independently and still required `selection["runner"] == "pytest"`. A script
target that legitimately reached PASS was therefore rejected by the consumer as
self-contradictory:
validation verdict contradicts its stages/findings: expected INCONCLUSIVE, got PASS
Two of the three places that encode the contract were updated and the third was not,
so the change silently made the evidence unusable at exactly the point it was meant to
be consumed. The coverage-basis check immediately above already handled `script`, which
is what made the omission easy to miss.
Verified against the report from #4538 on gfx950 (script target, --shape-arg
-s, PASS 9/9): rejected before, "validation report accepted for head 2285090..." after.
tests/test_validator.py 25 passed.
cagrikymk
added a commit
that referenced
this pull request
Sep 2, 2026
Two independent launcher-gated changes to the gfx950 fp8_mqa_logits kernel,
both found by profiling it with an ATT capture and hardware counters. The
kernel is matrix-core bound -- the MFMA runs at the full CDNA4 FP8 rate and
wastes nothing on tiling -- but the matrix core sits idle 37% of the time on
64 q-heads and 52% on 32, behind s_barrier, s_waitcnt and MFMA-hazard s_nop.
Both changes buy back some of that idle.
RELAXED_STORE (32 q-heads). With BLOCK_M=2 the loop walks the union of both
rows' KV ranges, so every store is masked to the part its row owns: 14 VALU
and 8 SALU per two KV tiles, on every tile, when the mask is all-true on all
but the last one or two. clean_logits=False already tells the caller that
out-of-window positions are theirs to fill or ignore, so there the mask
protects nothing and the multi-row store can collapse to the single-row one.
The union bound stays -- that is what keeps the store inside the row.
Docstring change: clean_logits=False now says those positions are
*unspecified* rather than untouched. A caller that wants -inf there must
fill it in after the call, not before. Both existing test suites that use
clean_logits=False already rehydrate that region themselves.
num_warps=1 + BLOCK_KV=32 (64 q-heads). A one-wave workgroup emits no
s_barrier at all. At num_warps=2 the async copy hands warp w the odd/even KV
columns while the MFMA layout has it consume a contiguous half, so the two
genuinely alias and Triton has to synchronise every tile -- 8.9% of wave
time, 100% stall. Halving BLOCK_KV keeps the per-wave work identical. The
cost is half as many waves, so it is gated on seq_len > 4096: measured 0.98x
at 4096 and 1.02-1.06x from 8192 up. BLOCK_M=2 halves the grid again, so
32 q-heads stays out.
Measured, warmup 300 then the median of five 500-iteration windows, arms
interleaved (the part is power limited, so short best-of-N windows are not
comparable):
64 q-heads 32 q-heads
1x4kx4k 1.000x (gates off) 1.001x (gates off)
1x8kx8k 1.026x 1.045x
2x8kx8k 1.046x 1.044x
4x8kx8k 1.021x 1.034x
1x8kx32k 1.058x 1.044x
2x8kx32k 1.032x 1.043x
Peak 2314 TF/s and 1967 TF/s, from 2242 and 1887. Against FlyDSL PR #4538 the
kernel goes from behind at two cells and level at three more to ahead
everywhere the gates fire: 1.04-1.15x at 64 q-heads, 1.01-1.13x at 32.
Verification: clean_logits=True compiles to a byte-identical instruction
stream to before (1287 instructions, only DWARF line numbers move);
clean_logits=False is bitwise identical in-window to the masked kernel and
deterministic run to run at six varlen layouts, including two where a
BLOCK_M=2 block straddles a batch boundary; 149/149 unit tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jhinpan
added a commit
to jhinpan/aiter-fork
that referenced
this pull request
Sep 2, 2026
Applying validate-kernel-pr to ROCm#4538 (a rewritten gfx950 FP8 MQA logits kernel) returned PASS in 93 seconds. The PR has a reproducible correctness failure -- flydsl_fp8_mqa_logits asserts at num_heads=16 on gfx950, where the pre-PR base computes it correctly -- and its central performance claim went unmeasured. Every gap below was found by running the tool, and each fix owns a generic invariant rather than that PR's shape. A shape grid that duplicates the target's own defaults is not a control. All three requested cells were already in the target's `--shapes` default, so the "independent" grid re-ran a strict subset of correctness_repo_tests and was credited `pass` -- the exact duplication SKILL.md says the stage exists to prevent. Grid cells are now compared against the flag's declared default; `test_selection.grid_independence` reports the outcome, and a passing duplicate is downgraded to `skip`. A duplicate that FAILS keeps its `fail`. A coverage axis the shape flag cannot reach could not be requested at all. `--grid` is one ordered tuple on one channel. A target whose head counts, dtypes or window modes live on separate flags had entire configurations unreachable however the grid was spelled. `--axis NAME=FLAG:v1;v2` adds them, under the same burden of proof as `--shape-arg`: the flag must be declared in the target's own argparse AND observed refusing a deliberately invalid value. An axis that fails either is dropped and NAMED, never dropped quietly. A script target's `executed: 1` stood for 56 graded cases. It is the same number a silently-returning target produces, and it earned the same `arch_coverage: runtime` on basis `script-exit-zero-with-output` -- a statement about the process, not the kernel. `stats.observed_work` now carries route calls counted in that run's own receipt, and a script run that observed no work earns no architecture credit. head-repo and head-grid shared one execution receipt. The second erased the first, and with the grid cells a subset of the defaults a receipt written by either run satisfied `--grid`. One receipt per run; `execution_receipt.receipt_scope` names the one that was published. "The PR adds this target, so base has nothing to time against" is not true. The file being new does not make the code it drives new. When the target only exercises an entry point that exists on base, the target is transplanted into the base tree and times the pre-PR implementation through the same harness. Because that spans two trees, `--perf-control-column` names a column the patch does not touch and the stage skips unless it reproduces within PERF_CONTROL_TOL. On ROCm#4538 this yields 56 matched rows, the kernel column 1.29x faster on head, and the untouched Triton control within 0.0%. scrape_perf matched 0 of 56 comparable rows. A header with no recognised unit was treated as identifying, but aiter bench tables print unlabeled MEASUREMENTS (`flydsl rel`, `triton err`, `speedup`) that differ between base and head by construction. The strict key is still tried first; only if it matches nothing is a relaxed key used, dropping identity columns whose cells are non-integral. `row_key_basis` records which. The target ran with the reviewer's environment attached. `env VAR=... cmd` adds to the inherited environment, so unmerged third-party code could read every token in the calling shell -- reproduced, with real credentials reaching a stage log. The target now runs under `env -i` plus a name/prefix allowlist and a secret-shaped denylist, and `isolation.target_environment` reports what was passed through. The exit code was read back out of `--out`. That made a previous run's report a fallback source of truth: a run killed before finish_report exited on the earlier run's verdict. `--out` is removed at startup and the code comes from a verdict file inside this run's own $WORK. Ten regression tests, each verified to fail against a8d46dc and pass here; the suite is 62 and green. The synthetic picker moved off a real device index, which was making the suite contend with genuine validations on the same host.
jhinpan
added a commit
to jhinpan/aiter-fork
that referenced
this pull request
Sep 2, 2026
A second script-target case, ROCm#5081, was picked after the first held-out round showed that the mechanisms added here had only ever executed their positive path on ROCm#4538. Selection was structural and outcome-blind ("an aiter PR that ADDS an op_tests script target with a shape flag and a sibling axis flag"); it immediately produced a false blocker. "Defines a test* function" is not "pytest can collect it". aiter's dominant op_tests convention is a SCRIPT whose worker happens to be named test_<op>(m, d, dtype) and is called from main() with real arguments. pytest collects it, cannot supply the parameters, errors -- and the validator published "the PR adds this test target and it fails on head" against an author whose target is green as a script with the very shapes the run had requested (verified by hand: -m 97 513 -n 512, all checkAllclose passed). A test* function is now only counted as a pytest node when it takes no required positional parameters or carries a parametrize/fixture decorator. ROCm#5081 selects `script`, the grid and the axis reach it, and no blocker is raised. `--runner` lets a caller settle the case the classifier still gets wrong, recording both the forced choice and what selection had said. A route behind functools.wraps could never be observed. A frame's identity is f_globals["__name__"] + ":" + f_code.co_name, and wraps copies __name__ onto the wrapper object while leaving co_name alone -- so aiter's whole @compile_ops family executes as `aiter.jit.core:wrapper` and naming the op matched nothing. The probe now resolves the declared route to code objects through the __wrapped__ chain; the string match remains as a fallback for a module that cannot be imported. On ROCm#5081 the receipt goes from observed '' to a real observed route, and INCONCLUSIVE now rests on the honest remaining gap (a (*args, **kwargs) dispatch wrapper binds no shape locals, so the receipt reports missing shapes rather than passing). Two fields answered the same question differently. When a proven axis rescues a duplicate shape grid, the override reached stages.correctness_s1_grid.independence but not test_selection, which had been written earlier -- so one report said "duplicates-target-defaults" and "adds-coverage" at once. Both are now written from the same decision. A control-gated perf stage still published the numbers it had rejected. `status: skip` beside a median_ratio and a regressed_rows list reads as a regression that was merely not acted on. When the control column moves outside PERF_CONTROL_TOL the ratio fields are dropped. Verified on ROCm#5081 end to end: novel grid -> grid_independence adds-coverage with axis `-n` proven (target_defaults 384/768/1024/4224, novel 512/1536); duplicate grid + no axis -> the stage downgrades to skip and both fields agree; duplicate grid + proven axis -> rescued, and they still agree. Seventeen new regression tests, all verified to fail against a8d46dc; the suite is 68 and green.
zufayu
pushed a commit
that referenced
this pull request
Sep 2, 2026
…des #4870) (#5142) * feat(skills): add validate-kernel-pr, a runtime validation layer for kernel PRs review-pr is static: it never builds and never runs. Three failure modes are invisible to it, and a 14-PR backtest of review-pr alone found 8 of 17 known defects, so the gap is real rather than theoretical. This skill produces validation_report.json as the evidence base: merge_sim -> gpu_claim -> runtime_compat -> test_policy -> correctness -> index_width_scan -> verdict Two stages exist because a green test suite is not evidence: - correctness runs the PR's own tests AND a shape grid this skill owns (non-toy, boundary/odd, long-context). Seeding a dropped tail guard into a FlyDSL softmax kernel leaves the repo's own suite green -- its non-aligned shapes are commented out, only one aligned shape runs -- while the grid at N=2000 / N=257 fails it. - test_policy compares tolerances head-vs-base before running anything. A change that widens a tolerance while leaving the kernel path unchanged makes the suite unable to fail, and pytest still reports green. Verified against three seeded mutants plus a clean baseline: baseline passes every stage; dropped tail guard blocks via the grid alone; loosened tolerance blocks via test_policy alone; wrong index unit blocks via the repo tests. The report states what it did not do, so it cannot overclaim by omission: arch_coverage marks each arch runtime or compile-only (a gfx950 host cannot validate a gfx942 claim), isolation records the real level, and degraded_mode marks COMPILE_ONLY when no GPU was claimable. runtime_compat exists because a pinned prebuilt runtime drifts behind the tree and the resulting ImportError otherwise reads as a defect in the PR. scan_index_width.py ships alongside it; see the review-pr D9 change. * fix(skills): make review-pr's D9 trigger structural, and require validation evidence Four changes, each from a measured failure in a 14-PR backtest of this skill. 1. D9 never fires. Its trigger was a list of variable names (token_id, seq_start, batch_offset, total_tokens). Three real 32-bit overflow defects used none of them -- stride_out_batch, block_id, physical_block, context_kv_idx -- and int32/int64/overflow appears zero times in the whole verdict card for aiter#1674, which carries two of them. The trigger is now structural: an index-shaped value multiplied by a stride-shaped value on a line with no 64-bit widening. scan_index_width.py is the mechanical pre-filter and flags all three; a bare model missed the same family, so this is not something to leave to recall. 2. REPO was hard-coded to ROCm/aiter, so the skill could not review a FlyDSL PR at all. It now takes a repository argument, or owner/repo#N. 3. FlyDSL/Triton kernel PRs now require a validation_report.json and load validate-kernel-pr. A kernel PR's own green suite is not evidence. 4. The verdict card caps at 5 findings, ordered most-severe first, and states its validation evidence. This skill averaged 7.1 findings per PR across the backtest; dropping everything judged noise still left 5.4, so the cap has to be explicit. Without a report the card says [static-only review], and no finding may then assert runtime behaviour as fact. Backtest numbers for the frozen version being changed here (blob 3fdb11af, 14 PRs, 17 provenance-checked labels): 76 of 100 findings useful, 0 factually wrong, 8/17 defects found. * docs(skills): correct validate-kernel-pr's documented interface The Invocation section showed `validate_pr.sh --pr 4394`, a flag the script does not implement -- it exits 2 with "unknown arg --pr". That is the hallucinated-API failure review-pr Step 6 exists to catch, shipped in the skill meant to catch it. Document the interface that is actually implemented and verified: the caller supplies the worktree and the pytest target. Every flag named in the doc is now present in the argument parser. Also declare what is absent rather than leaving it to be inferred: - no PR fetch orchestration; choosing the right --tests target from a diff is the unsolved part, and a wrong target produces a confident green - perf and claims stages are reserved in report_schema.json and emitted by nothing, so a report today carries no performance evidence and a review must not read a missing perf stage as "no regression" * fix(skills): run the D9 index-width scan in Step 1, not mid-checklist A scan that Step 5 asks for does not happen. In a controlled 14-PR run of the revised D9 text, no session invoked the scanner and the arm caught 0 of 3 known overflow defects -- the same as the unrevised skill. Moving the invocation into Step 1's fetch block does make it run: the session transcript for aiter#1674 shows the scanner executing and its candidate list entering the context. D9 now works that list instead of asking for a scan. This is necessary but NOT sufficient, and the PR text says so: with the scan in Step 1 the arm still caught 0 of 3. What closes the gap is supplying the production-scale facts the 5xx-gate demands -- see the limitations section. * fix(skills): attribute test failures against a baseline control Found by running validate-kernel-pr end to end on a real PR for the first time (FlyDSL#969, conv3d fp8 padded-M store guard). Both the pre-fix and post-fix trees failed the same test shape -- one unrelated to the PR -- and the harness reported "the PR's own test target fails" as a blocker in both. It charged pre-existing red to the author, which is the misattribution this skill exists to prevent. With --patch, the suite now runs on the base first and a failure is only a blocker when the base is clean; otherwise it is a note saying the target is red with and without the change. The same run goes from BLOCK to PASS with the pre-existing failure recorded rather than blamed. Also carries the production-scale table as a separate file printed by Step 1 beneath the scan candidates. Its effect is documented in the PR description and is small: 1 of 3 on the target defect family, versus 0 of 3 without it. * feat(skills): add downstream-impact-check, built from a real cross-repo incident Downstream tests here are label-gated and skipped by default, so the question "can this break vLLM or SGLang" decides whether they run -- and it is normally answered from memory. aiter#4530 is why that is not good enough. It changed one file, aiter/ops/triton/_triton_kernels/moe/moe_routing/topk.py: kernel internals, no public signature touched. It broke vLLM's gpt-oss MXFP4 MoE on MI355 after the 0.1.16.post5 -> 0.1.19 bump (vllm#49361, test plan blank), needed a hotfix that names it as the companion fix (vllm#50859), and SGLang reverted its aiter pin the same week (sglang#32879). scan_downstream_consumers.py walks aiter's imports upward from the changed files and stops at the first module a downstream checkout imports. On #4530 it reconstructs the chain from the internal topk kernel through moe_routing and moe_op_gemm_a16w4 to vLLM's aiter_mxfp4_w4a8_moe.py, and correctly reports no reachability for SGLang, which does not import that subtree. The first version asked "did a public symbol change" and answered "no downstream impact" for that exact PR. Signature is the wrong question; reachability is the right one. That earlier criterion is recorded in the file so it is not reintroduced. Validated on one positive and one negative, which is thin -- the skill says so, along with the rest of what it cannot see: static imports only, five hops, the downstream's current main rather than the pinned version, and no ATOM checkout. Wired into review-pr Step 1 alongside the other scanners, because a check the reviewer has to remember to run does not run. * fix(skills): prevent false kernel validation clearance Separate advisory review output from deterministic validation, make every missing evidence path explicit, and preserve exact base/head attribution. Commit the pinned mutant corpus so these safeguards remain reproducible. * fix(skills): reject unexercised validation paths Treat shadowed FlyDSL source, unused shape-grid hooks, and base-run artifacts as inconclusive instead of allowing them to produce false validation clearance. * fix(skills): harden validation evidence boundaries Require an exercised shape-grid handshake, isolated base/head state, and runtime-backed architecture coverage so incomplete evidence cannot be accepted as a clean result. * fix(skills): bind reports to the live base Resolve the current base branch tip instead of trusting the historical PR merge base, so validation reports become stale as soon as their merge simulation does. * fix(skills): require observed execution evidence Prevent green pytest from becoming clearance unless validator-owned instrumentation observes the expected route and shapes, JUnit records real executions, and runtime identity is captured. Align report semantics with process exits and reject untrusted FlyDSL runtime rebuild claims. * fix(skills): support script validation targets Run script targets through their real entry points so successful non-pytest validation stays inconclusive instead of producing a false BLOCK. Ship the AMD GPU picker so the validator can discover an idle translated HIP device. * Treat GPU activity as optional and hand the worktree back clean amdsmi_get_gpu_activity was a hard dependency at three call sites, so a host where that single query fails degrades the whole run to NO_GPU even with idle GPUs available. @zufayu hit this on MI308X / ROCm 7.0, where the call returns AMDSMI_STATUS_UNEXPECTED_DATA (43) while enumeration, BDF, ASIC and VRAM queries all work. PICKER could not route around it because the executor re-queries the API inline rather than going through the picker's selection. Activity is now optional. A new read_activity() helper is shared by the picker and both inline probes, and gpu_claim.idleness_basis names the evidence the claim rests on: activity+vram, or vram-only when only resident VRAM separated the devices. Unknown stays distinct from zero -- the previous `gfx if isinstance(gfx, int) else 0` substitution reported an unavailable metric as a measured idle GPU. The gpu_claim skip also stops conflating "GPUs present but none idle", an environment fact, with "AMD SMI is unqueryable", a portability gap in the validator. The import fallback did not probe $ROCM_PATH/share/amd_smi, where the bindings ship on ROCm >= 7.1, so on those containers the picker exited 2 before reaching any activity query -- the same NO_GPU symptom from an unrelated cause. Separately, merge_sim applied the patch but cleanup was guarded by BASE_ACTIVE, which is set only inside the baseline path. A run that skipped correctness exited with the patch applied, and when the baseline path did run, cleanup's restore_head re-applied it, so every path left the caller's worktree patched and the next run reported not isolated-clean against the caller. The application now carries its own flag and is reverted on exit, including on interrupt. Reported by @zufayu on ROCm/aiter#4870. * Let script targets carry grid and receipt evidence A target driven by a `__main__` guard could reach neither correctness_s1_grid nor execution_receipt, so INCONCLUSIVE was the hard ceiling for every aiter `op_tests/*.py` -- the repository's standard test shape. Neither skip described a property of the target: * the route profiler is sys.setprofile plus a receipt write, and pytest was only where the hook happened to be installed; * the grid was injected solely through an environment variable, while these targets take shapes on the command line. That is a limit of the injector. Reported as skips, both read as honest abstention while actually hiding a capability gap -- the failure mode this skill exists to prevent. The probe is now installed for script targets by run_script_with_probe.py, which executes the file under runpy with run_name="__main__". It calls the probe's own hooks rather than re-implementing the profiling, so `producer` stays truthful and the tested PR still cannot forge a receipt. The probe module is generated whenever a route is named, not only on the pytest branch; without that the runner had nothing to import. --shape-arg names the target's own CLI flag for the grid. The flag is named by the caller rather than guessed, because a wrong guess appends argv the target silently ignores. The hook is held to the same standard of proof as the env-var channel: the flag must appear in an add_argument call, and the existing invalid-grid probe still has to make the target fail before the stage is credited. A receipt is now validated whenever a route was named, including when no grid was configured or the grid channel could not be established. Two branches used to abandon the receipt along with the grid, discarding evidence already collected. With no grid it asserts route execution and nothing about shapes, which is all it is then entitled to claim. report_schema.json relaxes the PASS constraint `test_selection.runner: const pytest` to `enum [pytest, script]`, since a script target can now supply both missing stages. shape_arg is declared but deliberately not required, so reports from earlier validators stay valid. Verified on ROCm/aiter#4538 (gfx950, MI355 OAM), base 78b9440e/891788643: * script target with --shape-arg -s: PASS 9/9, exit 0, review-pr consumable, receipt naming _auto_variant over the four injected production shapes * unaccepted --shape-arg flag: correctness_s1_grid skip, receipt still pass * seeded defect (fused -inf fill dropping [e, seq_len_kv)): BLOCK, exit 1, with correctness_s1_grid failing independently of the PR's own suite * tests/test_validator.py 24 passed; PASS/INCONCLUSIVE/BLOCK reports and one pre-shape_arg report all validate against the schema * Ask the target whether it needs a GPU, and name what that answer rests on An unclaimable GPU suppressed all four correctness stages, so a target that needs no device reported nothing rather than reporting what it could establish. The target is now asked directly: when no GPU was claimed it is run once with no visible device, and test_selection.gpu_requirement becomes not-required only if it passes AND executes at least one test. The executed count is what makes this evidence rather than a guess -- a suite guarded by skipif(not torch.cuda.is_available()) also exits 0 having proved nothing, and a verified fixture covers that case. This is deliberately an observation, not a judgement about the diff. A Python-level dispatch change reroutes kernels without touching kernel source, and ROCm/aiter#5089 decides whether 34 gfx950 kernels compile from a seven-line helper, so no static rule over changed paths could settle it. Asking the target cannot be wrong in the dangerous direction: a target that needs a device fails or skips without one and stays required. PASS is deliberately NOT relaxed. `complete` still requires gpu_claim: pass, so this path cannot produce a clearance that was previously unreachable -- verified by a case where merge_sim, runtime_compat, test_policy, baseline_control, correctness_repo_tests, correctness_s1_grid, execution_receipt and index_width_scan all pass and the verdict is still INCONCLUSIVE with arch_coverage empty. gpu_claim stays `skip` because no device was claimed, which remains the fact; only the suppression is lifted. Two notes on scope, because this is a contract change rather than a defect fix. It edits behaviour @zufayu explicitly did not ask to change ("the gate is right and INCONCLUSIVE is the correct output"), and it changes an invariant the suite asserted, so test_no_gpu_is_inconclusive_and_every_skip_is_declared was updated and test_no_gpu_withholds_correctness_from_a_target_that_needs_a_device added to keep the original protection under test. It also does not by itself let a shape-less bugfix reach PASS: correctness_s1_grid has no passing path without a consumed grid. Also splits the last conflation in the gpu_claim skip. A host with no GPUs now exits 3 with "this host reports no GPUs" instead of borrowing the activity-unavailable wording, which blamed the validator for an absent device. Co-authored-by: Cursor <cursoragent@cursor.com> * State the promotion bar in review-pr's header @zufayu asked for this in item 4 of ROCm/aiter#4870: the bar belongs in the header because a header is read on every use while an issue sinks. #5128 was opened for the measurement work and closed as premature, so without this the condition for ever leaving advisory tier is recorded nowhere. Half of it was already present -- "its judgement is stochastic and never blocks a merge". What was missing is the bar itself: the tool stays advisory until false clearance is measured and near zero for the families that raise a red verdict. That number is named as the one that matters, distinct from recall or a spot check, and stated not to exist today because no committed replay corpus establishes it. Ownership is attached to whoever proposes a rule edit or proposes gating, which is where the trigger actually is. Also completes the item-4 headline ask. The report was already optional in the code (`VALIDATION_REPORT="${3:-}"`, the no-report branch, "absence of a report is not itself a blocker", and Step 8's NOT RUN line), so nothing gated on it. The one place that read as mandatory was the front-matter description, which said to invoke with "an explicit validation report path"; it now says a review without one is a supported outcome. Takes the second option offered for exempt target classes and names them on the FlyDSL kernel row, because the combination is what item 4 objected to: a CPU-only target claims no GPU and therefore no architecture, and a bugfix with no shape dimension has no grid for correctness_s1_grid to consume. Both are structurally unable to reach PASS, so their INCONCLUSIVE is the expected output and that author must not be asked for a passing report. Co-authored-by: Cursor <cursoragent@cursor.com> * Let review-pr accept a script target's PASS `Let script targets carry grid and receipt evidence` relaxed the PASS-requires-pytest assumption in the validator and in report_schema.json, but review-pr re-derives the verdict independently and still required `selection["runner"] == "pytest"`. A script target that legitimately reached PASS was therefore rejected by the consumer as self-contradictory: validation verdict contradicts its stages/findings: expected INCONCLUSIVE, got PASS Two of the three places that encode the contract were updated and the third was not, so the change silently made the evidence unusable at exactly the point it was meant to be consumed. The coverage-basis check immediately above already handled `script`, which is what made the omission easy to miss. Verified against the report from ROCm/aiter#4538 on gfx950 (script target, --shape-arg -s, PASS 9/9): rejected before, "validation report accepted for head 22850902..." after. tests/test_validator.py 25 passed. * Have review-pr triage validation need, and run it when it can A judgement the checklist asks for mid-review is a judgement that does not get made, so Step 1 now classifies the changed paths itself and, when the PR changes runtime surface and ships exactly one test target, invokes bin/validate-kernel-pr and consumes the result through the unchanged identity gate. Producing a fresh head-bound report is not the same act as adopting one found on disk, so "never auto-load ./validation_report.json" still holds. The verdict line gains the two states it was missing. A README fix used to report the same "NOT RUN" as an unvalidated kernel rewrite, which teaches a reader to skip the line; "N/A - no runtime surface changed" and "required but not run" are different facts and now read as such. Co-authored-by: Cursor <cursoragent@cursor.com> * Call the validator from the skill, not from bin/ The auto-run reached outside .claude/skills for bin/validate-kernel-pr, whose only addition over validate_pr.sh is a PR-number front end: it re-fetches the diff and re-resolves the base tip. Step 1 already holds both, so it now creates a detached worktree at the base it recorded and hands that, its own pr.diff and the head OID straight to the validator. Deriving those a second time was not merely redundant. main can advance between the two gh api calls, and the resulting report names a base the review's own identity gate then rejects as stale -- a self-inflicted failure on a report we just produced. Handing over the recorded base makes repo.base agree by construction, since the validator reads it as rev-parse HEAD of that worktree. The dependency also now sits beside the index scanner and the report schema, and fails loudly like they do instead of warning and continuing, since a skill tree missing one of the three is broken rather than differently configured. Finally, each way the run can give up records why. Triage's blocker only covers attempts never made, so a run that was attempted and failed printed "required but not run: None" -- the exact uninformative verdict line this step exists to remove. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(skills): make the D9 index-width scan an AST pass with no name lists The scanner matched a regex over raw diff text whose operand character class contains neither a comma nor a space, so it could not span a broadcast subscript -- and ptr[:, None] * stride is the standard Triton pointer-arithmetic idiom. Measured on aiter#4978, the PR that introduced the moe_wgrad overflow later fixed by #5132: both real defect lines were missed, the one moe_wgrad candidate emitted was an already-int64 line, and 390 candidates were produced overall. The defect lived in the syntax tree; the scanner was reading text. It also matched operand names against two hard-coded lists. Both are gone. A multiplication is now a candidate when it feeds an addition chain, at least one operand derives from a non-constexpr parameter of the enclosing kernel, and no operand is widened -- counting a widening carried in from an earlier line, which is how #5132 fixes it. Post images come from --source-root, else the diff's own index blobs; a file whose post image cannot be recovered is reported as NOT SCANNED, never dropped, because an empty candidate list that silently excluded files is not evidence of absence. On #4978 it now reports 4 of 4. On #5132 it reports none. review-pr's D9 text claimed a structural trigger while still defining index- and stride-shaped by name; it now describes what the scanner actually does. * feat(skills): deliver the S1 shape grid through pytest parametrization Applying this validator to four open aiter FlyDSL kernel PRs (#5167, #5141, #4992, #5011) reached INCONCLUSIVE on all four, for one reason: correctness_s1_grid skipping is on its own enough to force it, and the stage is inert on this repository's dominant test shape. Zero of the seven files in op_tests/flydsl_tests/ read a shape env var or parse a shape CLI flag; every one declares its shapes as literals inside @pytest.mark.parametrize. --shape-argnames adds parametrization itself as a third channel, via a generated plugin whose pytest_generate_tests substitutes the grid. The burden of proof is unchanged: a deliberately unusable grid must still make the target fail, so a target that ignores the grid produces a skip and never a pass. The invalid-grid row keeps its arity and poisons its values, so the probe fails inside the target rather than inside the plugin. A mark binding the requested names together with others is refused rather than stripped: removing it would leave those others as unfilled fixtures and the target would error for a reason unrelated to the kernel. Two channel defects found alongside it. The env probe overwrote the CLI probe unconditionally, so supplying both flags discarded a working CLI channel and then reported the env channel's absence as the reason no grid ran; the probes are now independent and run_pytest dispatches on the channel that actually probed positive. And the skip text read 'kernel exposes no configured shape override' even when the cause was the validator ignoring the named channel -- grid_channel_reason now names each channel tried, what was found, and what the target does offer instead. * fix(skills): stop reporting pass on evidence that was requested and never collected Three stages decided their status from 'this code path completed' rather than 'evidence was actually collected', and published pass on empty fields. execution_receipt: the profiler sampled f_locals only on the call event, so it saw parameters and nothing else. A route deriving its shapes in its body -- the common case for a kernel whose arguments are tensors -- could never satisfy --shape-vars, and the receipt reported pass with executed_shapes: []. Observed on three separate PRs. The frame is now sampled again on return, keyed by the frame OBJECT rather than id(), whose values are reused once a frame is freed; the first version of this fix used id() and dropped the second of two sequential calls. When capture was requested and produced nothing, the receipt now carries a shape_capture block saying the receipt asserts route execution and makes no claim about shapes. test_policy: the tolerance pattern matched only literal atol=/rtol=, so a file declaring DEFAULT_REL_TOL = 2e-2 reported zero tolerances and the check passed on an empty list. Loosening a named constant is exactly the m2 mutant this stage exists to catch, and it was undetectable. Named constants are now captured, and a tolerance passed by name is recorded so an empty list is never read as 'none'. * fix(skills): claim a GPU that is actually free, and name the basis honestly PICKER resolved from PATH before the picker this skill ships. The shipped picker is what prints idleness-basis:, so a PATH copy that omits it won by default and the report published idleness_basis: 'unknown' beside a concrete gfx_activity_before_pct: 0 -- an unavailable reading presented as a measured idle one, which SKILL.md argues against in its own text. Resolution is now explicit PICKER, then shipped, then PATH. The picker is deterministic and returned one device, and a contended flock -n gave up. Concurrent validators therefore all selected the same GPU and all but one reported no idle GPU while seven sat idle; observed twice in one run with every lock free between attempts. The picker gained --all, and the executor walks the eligible ranking until a lock is taken. * test(skills): cover the validator changes Thirteen tests for the AST scanner, the three grid channels, receipt honesty and the GPU claim. Verified against the pre-fix sources: 5 of 6 scanner tests and all 8 of the executor tests fail there, each for the intended reason. The exception is the earlier-line-widening test, which passes pre-fix coincidentally -- the old regex also failed to span token_row[:, None], for its own wrong reason. test_json_count_is_deduplicated is rewritten rather than restored: it asserted the name-list contract that was deliberately removed. * fix(skills): stop the validator from publishing its own faults as the PR's Round-2 application to five real FlyDSL kernel PRs found six ways a fact about the run was published as a defect of the code under test. Three were introduced by the pytest grid channel itself. --shape-argnames with exactly ONE name passed argnames as a list while unwrapping rows to scalars. pytest sets force_tuple only for a string argnames, so collection died with "object of type 'int' has no len()" and the executor published that crash as 'the PR adds this target and its independent shape grid fails' -- a BLOCK verdict against the author, on three separate PRs. One name is the dominant shape in the targets this channel was built for. The partial-overlap guard was evaluated over the whole FILE, so an unrelated test whose parametrize mark overlapped the requested names disabled the channel for every test in it. The plugin decides per metafunc; the probe now decides per test function, as it always should have. A relative --patch was resolved against the caller's cwd in one place and the worktree in another, so merge_sim reported 'patch does not apply to the current base' and returned BLOCK. Paths are resolved once, before anything moves. head-repo and head-grid shared one receipt path through their phase directory, so a grid run that crashed during collection erased a receipt that had already proved the route, and the report then said the route never ran. Receipts are per label, and one helper decides which speaks for the head run: a phase that observed nothing never speaks over one that observed something. The invalid-grid probe treated any non-zero exit as proof the grid was consumed. On a held-out PR whose module could not be imported, and again when the plugin itself crashed, the channel was credited although no shape reached the kernel. The control run must now pass before the probe's failure means anything. JUnit errors were counted as executed tests, so a collection error credited arch_coverage with runtime coverage on the strength of the error itself. WIDEN_ATTRS was matched case-sensitively; FlyDSL writes fx.Int64(...), so explicitly widened FlyDSL code was reported as an overflow candidate. Found only on the held-out PR -- the four PRs used while fixing all happen to write tl.int64. Two long-standing gaps closed alongside them. --tol-table was parsed, validated and compared against nothing; it now yields a finding when the suite accepts error beyond the loosest reference supplied. And runtime_identity published native_artifacts: [] as a measurement when the probe runs before the target and imports only aiter -- it now states what that emptiness does and does not mean. The pytest channel delivers the grid as test PARAMETERS while the receipt records the ROUTE's locals. Requiring containment across those two vocabularies produced a missing-shapes skip on a run whose every grid case passed; the requirement is recorded, the cross-namespace assertion is not made, and review-pr's ingestion gate knows the difference. * test(skills): cover the false-attribution fixes Twelve tests for the second batch. All twelve fail against the pre-fix sources and none pass there, so each one discriminates. The single-name --shape-argnames test asserts the injected values reach the route rather than the target's own literal, which is what the false BLOCK hid. One needed the caller's directory nested two levels rather than one: with a single level, a relative --patch resolves to the same file whether it is read from the caller's cwd or from the worktree, and the defect is invisible. The fixture's synthetic HIP index moved from 7 to 57. The validator flocks /tmp/gpu-<index>.lock, and an unrelated job on this host holds the lock for 7, so every GPU-claiming test degraded to NO_GPU for a reason that had nothing to do with the code. No assertion was weakened; only the synthetic device number changed. * fix(skills): gate the pytest grid on parametrize VALUES, not just argnames Round 3 found the false BLOCK on #5141 was not gone. The plugin crash was fixed; the crash had moved into the author's file, which makes the misattribution harder to see rather than less real. The gate read a parametrize mark's argnames and never its values. A target parametrizing a single `case: dict` therefore passed it, the validator substituted integers, and the target raised `TypeError: 'int' object is not subscriptable` -- published as '[blocker] the PR adds this target and its independent shape grid fails', against an author whose own 138 tests passed in the same report. SKILL.md already documented that such targets stay INCONCLUSIVE; the executor did not implement it. Both the AST gate and the plugin now require the mark's own values to be scalar cells, and values of unknown shape count as not scalar -- a grid this channel cannot express must cost an INCONCLUSIVE, never a blocker aimed at an author. Three more false attributions closed with it. review-pr asserted `executed == tests - skipped` while the validator had been corrected to subtract errors. They disagree only when errors > 0, which is the shape of a report carrying a real runtime blocker -- so a report that had correctly found one was rejected at ingestion and could not be used. Worse than the overclaim it replaced, and caught only by re-running the case that had produced a true BLOCK. The invalid-grid probe's causality guard had been added on the base side only, so the head branch still credited the channel when the target failed for an unrelated reason. And review-pr turned a grid the caller supplied into a requirement even when no channel had carried it, so the receipt's honest empty list read as a contradiction and discarded exactly the runs carrying the accurate diagnostic. The scanner claimed kernel scope in its docstring and its class name while visiting every Python function: on a held-out PR all four candidates were host-side float FLOP accounting and none were in the 916-line kernel. Device scope is now a predicate -- a constexpr-annotated parameter, or a jit/kernel decorator -- and host-scope hits are listed separately rather than dropped, because that predicate is a spelling test and a wrong answer should cost a glance, not a miss. The index-width scan now reads post images from the applied worktree, so modified files are examined rather than disclosed as unscanned. gpu_requirement no longer reports a conservative default in language that reads as an observation. * fix(skills): scope the scan to the enclosing kernel, and reject a caller's typo at the door Two defects found by re-running the cases that had produced decisive verdicts. A grid whose rows did not match --shape-argnames produced a BLOCK. The arity check lived inside the plugin generator, whose exit status run_pytest never read: the generator failed, the PREVIOUS phase's plugin file survived on disk, head-grid re-ran the invalid-grid sentinel it still carried, and that failure was published as 'the PR adds this target and its independent shape grid fails'. A check whose result nobody reads is worse than no check -- it creates the appearance of a guard while the system falls back to stale state. The arity check now runs at argument parsing, where a caller's mistake cannot become a finding about the PR, and the generated plugin is removed before regeneration so no phase can inherit another's. The index-width scan was classifying real kernel code as host-side. _scan_body used ast.walk, which descends through nested defs, so every expression inside a nested helper was scanned in the ENCLOSING function's scope: its parameters, its widened locals, its device/host verdict. Four candidates inside a @flyc.kernel body were demoted to host scope, and a miss matters more here than noise. The walk now stops at function boundaries, and scope, parameter provenance and widening follow the scope chain -- a nested helper closes over its kernel's parameters, so it inherits them. Fixing that exposed a second bug it had been masking: visit_FunctionDef recursed with generic_visit, which visits a statement's CHILDREN, so a nested def that IS a statement of the body was never dispatched at all. Evidence unchanged where it should be and corrected where it was wrong: aiter#4978 still reports 4 of 4 on the moe_wgrad overflow, #5132 still reports none, #5141's nested-kernel arithmetic moves from 4 host to 5 device, and the held-out #5025's host-side FLOP accounting stays out of the device list. Two abstentions also stated the wrong cause -- 'does not take all of these as test parameters' for a target that does take them, when the real refusal was non-scalar mark values. The probe now returns the refusal it actually made. * fix(skills): let auto-validation reach the pytest shape channel The auto-validation block added on this branch forwards REVIEW_SHAPE_ENV and REVIEW_SHAPE_ARG but not the parametrization channel, so the one channel that reaches this repository's dominant test shape was unreachable from the caller that was just wired up. None of the seven files in op_tests/flydsl_tests/ reads a shape env var or parses a shape flag; all declare their shapes as literals in @pytest.mark.parametrize. Also records where a caller learns which channel a target actually offers, so a wrong guess costs one run rather than a reading of the target's source. * Measure a kernel PR's cost, not just its correctness A kernel PR could pass every stage while running slower, because nothing timed it. Add a `perf` stage: bench the target once with the patch reversed and once with it applied, both inside the base phase's lock on the same GPU, and compare. The comparator (scrape_perf.py) reads both of aiter's timing formats -- the `df.to_markdown()` tables from `@benchmark`/`run_perftest`, and the older `[perf] ... ck avg: N us` lines that 11 kernel targets print. A scraper that only knew tables would burn a full base+head run on those and report nothing. Three choices worth recording, each forced by a measurement rather than by taste: - Take the *minimum* over per-column medians, not the mean. A PR touching the ck path leaves `torch avg` at 1.0, and a mean would let that untouched reference column bury the regression next to it. - Run each side three times and keep the best sample per cell. Five warm runs of unchanged test_layernorm2d.py gave `torch avg` 14.24 14.64 14.18 14.23 14.31 (1.03x spread) but `ck avg` 13.10 20.98 20.70 13.28 13.17 -- 1.60x, and bimodal rather than noisy. Single-shot at a 0.95 threshold would have called unchanged code a regression about half the time. Best-of-3 brings the same data to 1.014x, worst ratio 0.986. - Snapshot the worktree around the timing run and roll back only the paths it dirtied. Without this a bench that writes a results file (see op_tests/test_gemm_a8w8.py, test_moe_2stage.py) fails the base phase's cleanliness check, which sets BASE_READY=0 and silently skips head correctness entirely. Measured: the same target went PASS with --no-perf and INCONCLUSIVE with perf on. A perf stage that quietly disables correctness is worse than no perf stage. A regression under the threshold writes a should-fix finding, so the deterministic verdict becomes NEEDS_WORK and the process exits 1. review-pr's identity gate now cross-checks the stage's status against its own numbers, so neither a laundered pass nor a laundered fail survives, and its P6 rule fires only when no usable stages.perf exists. The harness detector is shared in spirit by both skills and pinned by a test, since a silent disagreement there means one skill demands a measurement the other cannot supply. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Describe the validator this branch actually ships The perf stage landed six commits ago and its own skill still says it does not exist: "the schema reserves both -- and the script emits neither. A report today carries no performance evidence, and a review must not read the absence of a `perf` stage as 'no regression'." That instructs a reader to disbelieve a stage that now measures, gates, and can turn a verdict into NEEDS_WORK. review-pr says the opposite in three places, so the two skills contradicted each other on the one stage a reviewer is most likely to act on. Document `perf` where the implementation lives: the flag table gains --perf-args and --no-perf, the env knobs gain PERF_TIMEOUT, PERF_REPEAT, PERF_THRESHOLD and PERF_MIN_ROWS -- all of them ${VAR:-default} overrides with their own argument validation, so they were public already and only undocumented -- and the stage list gains a section that says why best-of-N is what makes a 0.95 threshold usable and why every non-measurement path reports `skip`. The verdict section now states that PASS does not imply a timing comparison happened. What remains genuinely unimplemented is narrower than the old bullet claimed: reproducing the specific numbers a PR description states. That is now what "Not implemented yet" says. Three smaller corrections in the same vein: - "review-pr never builds and never runs" has been false since review-pr started invoking this script itself. The split is at judgement, not at invocation, and the text now says so. - The comment describing bin/validate-kernel-pr as an existing wrapper outlived the wrapper; bin/ is not in the repository. The reasoning it carried -- do not re-resolve a base that main can advance past -- is worth keeping, so it stays, phrased about a front end that does not exist rather than one that does. - The mutant manifest's tolerance_table disagreed with the file it describes: test_softmax.py at the pinned base uses f16=1e-2 and bf16=2e-2, not 2e-3 and 1e-2. Inert today, because reference tolerances are recorded and not compared, which is exactly how a wrong number survives unnoticed until something starts reading it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Stop the report schema promising evidence nothing produces report_schema.json is the contract review-pr validates against, so a field declared there reads as a shipped capability. Three did not hold. `claims` declared a stage that reproduces the numbers in a PR description, with an `unreproducible` array. No producer writes it -- not validate_pr.sh, not validate_evidence.py, not scrape_perf.py. Its only other mention was an allowlist entry saying it may be absent. It is removed rather than left standing; the honesty rule that promised `[unreproducible]` tagging now describes what `stages.perf` actually carries. `source_sha` was hardcoded null while the same honesty rules listed source SHA as recorded evidence. It is fillable: the resolved module has a path, and that path is usually inside a checkout. Now it records that checkout's commit, suffixed `-dirty` when the tree has uncommitted changes -- which the head phase always does, since it runs with the candidate patch applied, so the bare commit would name the base and describe a tree that is not the one under test. A module resolved from an installed package stays null: attributing a commit to a prebuilt binary is precisely the provenance claim this skill refuses to make. `reference_tolerances` is recorded and never compared, so --tol-table influences no verdict. Wiring it up is not possible as the data stands -- the table is keyed by dtype while the scraped tolerances are a positional list, with nothing to join them on -- so the field now says that it is context and not a gate, rather than implying otherwise by sitting next to the comparison that is one. Also collapses the PASS gate's two byte-identical correctness blocks into definitions/passing_correctness_stage, and has its `stats` $ref the execution_stats definition that was already there instead of respelling it inline. 74 duplicated lines in the clause that decides what earns a clearance, where divergence between the copies would change the answer silently. Verified equivalent: a clean PASS report still validates, and executed=0, failures=1, errors=1, exit=1, status=skip, missing stats and a stats object missing `skipped` are each still rejected on both stages. Adds the fields the producers already emit but the schema never described -- gpu_claim.post_run_note and .requirement_note, perf.regressed_rows[].base_repeats and .head_repeats -- which passed only because additionalProperties defaults to true. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Name the channel the shape grid actually travelled on --shape-arg was added so script targets could receive an S1 grid, but the code around it still assumed the grid could only be an environment variable. Three consequences, one of them visible in a report. GRID_HOOK_OK was assigned twice. The --shape-arg AST scan set it, then an unconditional second block re-derived it from the --shape-env scan. Supplying both flags therefore proved the env var while run_pytest put the shapes on argv -- the report attested a channel the run did not use. Only-one-flag runs worked, which is why this survived. The channel is now resolved once, before either scan, and --shape-arg wins for a script target because that is what run_pytest does. A --shape-arg naming a flag the target does not accept fell through to the branch that reports "no shape grid was configured". Both cases skip, so the reason is the only thing distinguishing a validator that could not find the hook from a caller who never asked for one -- and the wrong one of those is an environment fact standing in for a capability gap, which is the overclaim-by-omission this skill exists to prevent. The branch conditions now test whether a grid was requested at all, so the same input reports hook-not-found, names the flag, and records test_selection.grid_channel. --shape-arg on a pytest target gets its own answer rather than the same silence: argv never reaches it. The runtime skips likewise said "shape environment variable" whichever channel was in play. They name the real one now. run_pytest took "$SHAPE_ENV=$GRID" and re-split on the first `=`. For a CLI-only run that worked only because an unset SHAPE_ENV left a leading `=` the split then removed, so the shapes rode inside a string shaped like the channel they were not using. It takes the grid value directly and picks the channel from the same decision that credited the hook. Collapses the two grid-less branches, which were a 17-line receipt block duplicated verbatim with two different skip strings. The new test fails on the previous script for the reason that matters -- 'no configured shape override' unexpectedly found -- not merely on the field added to carry it, which is why the note assertions come before the grid_channel one. 40 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Find the index-width overflow the D9 scan was written for The scanner matched a regex over raw diff text whose operand character class contains neither a comma nor a space, so it could not span a broadcast subscript -- and `ptr[:, None] * stride` is the standard Triton pointer-arithmetic idiom. It also matched operand names against two hard-coded lists. Measured on aiter#4978, the PR that introduced the moe_wgrad overflow later fixed by #5132: 390 candidates emitted, none of them the four real defect lines, and the single moe_wgrad candidate was `off_expert * stride_dwe` -- a line whose operand is widened with `.to(tl.int64)` forty rows earlier. Recall 0 of 4, on the defect family the rule is named for. The defect lived in the syntax tree; the scanner was reading text. Rewritten as an AST pass, taken from #5174. Both name lists are gone. A multiplication is a candidate when it feeds an addition chain, at least one operand derives from a non-constexpr parameter of the enclosing kernel, and no operand is widened -- counting a widening hoisted to an earlier line, which is how #5132 fixes it. Scope follows the scope chain rather than ast.walk, so a nested helper inherits its kernel's parameters instead of being scanned in the wrong frame. Post images come from --source-root or the diff's index blobs; a file whose post image cannot be recovered is reported as NOT SCANNED rather than dropped. It reports 4 of 4 on #4978 and 0 on #5132. Two defects fixed on top of what #5174 ships. AugAssign was never scanned. _scan_body matched ast.BinOp with an Add op, but `a_ptrs += BLOCK_K * stride_ak` is an ast.AugAssign, which is not a BinOp -- so the most common Triton pointer-advance idiom was a blind spot. The constexpr filter in _is_compile_time was written for exactly that expression and could never reach it. Adding it moves #4978 from 296 candidates to 306; the moe_wgrad four and the #5132 zero are unchanged. WIDEN_CALL_RE was left behind by the regex implementation this replaces and had no remaining reader. The --json path returned `0 if not unscanned else 0`. It stays 0, because both callers require that: #5174's own test asserts the plain and --json runs exit 0, and validate_pr.sh turns a non-zero exit into a skipped stage rather than a louder finding. Making the exit status honest needs validate_pr.sh to pass --source-root or to read the `unscanned` field, so the reason is recorded where the return is instead of a ternary whose two branches were the same value. Base's IndexScannerTests asserted the name-list contract that was deliberately removed, down to a JSON key that no longer exists; #5174's rewritten IndexScannerTests and ScannerScopeTests replace it. 48 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Stop the receipt reporting pass on shapes it never captured The probe sampled a frame's f_locals on the `call` event only, so it saw parameters and nothing else. A route that derives its shapes in its body -- the common case for a kernel whose arguments are tensors -- could never satisfy --shape-vars, and the receipt published `pass` beside `executed_shapes: []`. Deciding status from "this code path completed" rather than "the evidence was collected" is the failure this skill argues against in its own text, and it was in the probe. From #5174: the frame is sampled again on return, keyed by the frame OBJECT rather than id(), whose values are reused once a frame is freed and would drop the second of two sequential calls to the same route. When capture was requested and produced nothing, the receipt now carries a shape_capture block saying it attests route execution and makes no claim about shapes, so no consumer can read the empty list as evidence that no shapes were needed. The evidence checker stops counting errors as executed tests. Errors are collection and fixture failures where the body never ran, so a target that failed to import was crediting arch_coverage with runtime coverage on the strength of the collection error itself. This also makes the checker agree with review-pr's ingestion gate, which already asserts `executed == tests - skipped - errors`: they disagree only when errors > 0, which is the shape of a report carrying a real runtime blocker, so the two had to be corrected together or a correct report would be rejected at the door. runtime_identity stops publishing `native_artifacts: []` as a measurement. The probe runs before the target and imports only the module under test, so an empty list describes that import's reach and not the kernel's; the basis string now says which. validate_receipt gains a `--grid-channel` argument, used to suppress the cross-namespace containment assertion when the grid travelled as pytest parameters. It defaults to empty and validate_pr.sh does not pass it on this branch, so that branch is inert until the pytest grid channel lands. 50 pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> * Let the GPU picker rank every eligible device, not just one The picker returned a single device and the executor took a contended `flock -n` as a refusal. Concurrent validators therefore all selected the same GPU and all but one reported "no idle GPU" with seven sitting idle -- observed twice in one run with every lock free between attempts. A deterministic picker and a non-blocking lock are only safe together if the caller can fall through to the next candidate. From #5174: --all prints the eligible devices in ranking order rather than the top one. The executor side of this, walking that ranking until a lock is taken, lives in validate_pr.sh and is not in this commit, so nothing consumes --all yet; the default single-device output is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix(skills): stop the validator reporting coverage it did not obtain Applying validate-kernel-pr to ROCm/aiter#4538 (a rewritten gfx950 FP8 MQA logits kernel) returned PASS in 93 seconds. The PR has a reproducible correctness failure -- flydsl_fp8_mqa_logits asserts at num_heads=16 on gfx950, where the pre-PR base computes it correctly -- and its central performance claim went unmeasured. Every gap below was found by running the tool, and each fix owns a generic invariant rather than that PR's shape. A shape grid that duplicates the target's own defaults is not a control. All three requested cells were already in the target's `--shapes` default, so the "independent" grid re-ran a strict subset of correctness_repo_tests and was credited `pass` -- the exact duplication SKILL.md says the stage exists to prevent. Grid cells are now compared against the flag's declared default; `test_selection.grid_independence` reports the outcome, and a passing duplicate is downgraded to `skip`. A duplicate that FAILS keeps its `fail`. A coverage axis the shape flag cannot reach could not be requested at all. `--grid` is one ordered tuple on one channel. A target whose head counts, dtypes or window modes live on separate flags had entire configurations unreachable however the grid was spelled. `--axis NAME=FLAG:v1;v2` adds them, under the same burden of proof as `--shape-arg`: the flag must be declared in the target's own argparse AND observed refusing a deliberately invalid value. An axis that fails either is dropped and NAMED, never dropped quietly. A script target's `executed: 1` stood for 56 graded cases. It is the same number a silently-returning target produces, and it earned the same `arch_coverage: runtime` on basis `script-exit-zero-with-output` -- a statement about the process, not the kernel. `stats.observed_work` now carries route calls counted in that run's own receipt, and a script run that observed no work earns no architecture credit. head-repo and head-grid shared one execution receipt. The second erased the first, and with the grid cells a subset of the defaults a receipt written by either run satisfied `--grid`. One receipt per run; `execution_receipt.receipt_scope` names the one that was published. "The PR adds this target, so base has nothing to time against" is not true. The file being new does not make the code it drives new. When the target only exercises an entry point that exists on base, the target is transplanted into the base tree and times the pre-PR implementation through the same harness. Because that spans two trees, `--perf-control-column` names a column the patch does not touch and the stage skips unless it reproduces within PERF_CONTROL_TOL. On #4538 this yields 56 matched rows, the kernel column 1.29x faster on head, and the untouched Triton control within 0.0%. scrape_perf matched 0 of 56 comparable rows. A header with no recognised unit was treated as identifying, but aiter bench tables print unlabeled MEASUREMENTS (`flydsl rel`, `triton err`, `speedup`) that differ between base and head by construction. The strict key is still tried first; only if it matches nothing is a relaxed key used, dropping identity columns whose cells are non-integral. `row_key_basis` records which. The target ran with the reviewer's environment attached. `env VAR=... cmd` adds to the inherited environment, so unmerged third-party code could read every token in the calling shell -- reproduced, with real credentials reaching a stage log. The target now runs under `env -i` plus a name/prefix allowlist and a secret-shaped denylist, and `isolation.target_environment` reports what was passed through. The exit code was read back out of `--out`. That made a previous run's report a fallback source of truth: a run killed before finish_report exited on the earlier run's verdict. `--out` is removed at startup and the code comes from a verdict file inside this run's own $WORK. Ten regression tests, each verified to fail against a8d46dcb0 and pass here; the suite is 62 and green. The synthetic picker moved off a real device index, which was making the suite contend with genuine validations on the same host. * fix(skills): let the mutant replay reach its own verdict check The driver reverse-applied each case's patch unconditionally after the run. But validate_pr.sh already hands the worktree back with the patch reversed, so that second reverse-apply failed with "patch does not apply" on EVERY case, and under `set -euo pipefail` it aborted the driver before the summary block that compares each verdict against the manifest. The mutants were being discriminated correctly the whole time -- m0 PASS, m1 and m3 BLOCK in correctness, m2 BLOCK in test_policy -- but the suite could not say so, and its exit status was 1 regardless of the result. A regression asset that cannot report itself green is not one. Reverse only when `git apply -R --check` says the patch is still applied. End to end on ROCm/FlyDSL@421935cc with the installed flydsl 0.3.1 (verified identical to that commit's python/flydsl tree), the replay now prints "mutant replay matched all expected verdicts". Found by running the committed replay as a held-out case, not by reading it. * fix(skills): say which fact a skipped comparison rests on Three defects that only appeared on ROCm/aiter#5172, a fresh held-out PR frozen before any of this work started and never looked at while fixing. A requested axis vanished from the report when it could not be honoured. #5172's target is collected by pytest, so argv-borne axes cannot reach it and axis_state became "unusable" - correct - but test_selection.axes was published as [], so a reader could not see that an axis had been requested at all. An empty list beside a non-"none" state loses the request itself, which is exactly the silently narrowed test space these fields exist to make visible. Axes are now recorded as asked for, with hook_proof "not-evaluated" when the run never got far enough to scan the target for the flag. grid_independence published a false statement about the target. Its default reason, "the channel exposes no declared defaults to compare against", was emitted whenever the comparison did not happen - including when the channel had been demoted for an unrelated reason. On #5172 that is untrue: the target's -c flag does declare a default list, and all three requested cells were outside it. The reason now names which of the several skip paths applied. "Red on both baseline and head" is an attribution, not an explanation. #5172's target defines pytest nodes AND parses argv in its module body, so pytest imports it at collection with its own argv and argparse exits. The file is green run as a script - the perf stage in the SAME run proves it, three times per side. The report said the target was red and gave no hint that the runner selection was the cause. test_selection.runner_risk now names that structurally, and a run that executes nothing under the selected runner cites it. Thirteen new regression tests in all, each verified to fail against a8d46dcb0; the suite is 63 and green. * fix(skills): stop charging a runner-selection artefact to the PR author A second script-target case, ROCm/aiter#5081, was picked after the first held-out round showed that the mechanisms added here had only ever executed their positive path on ROCm/aiter#4538. Selection was structural and outcome-blind ("an aiter PR that ADDS an op_tests script target with a shape flag and a sibling axis flag"); it immediately produced a false blocker. "Defines a test* function" is not "pytest can collect it". aiter's dominant op_tests convention is a SCRIPT whose worker happens to be named test_<op>(m, d, dtype) and is called from main() with real arguments. pytest collects it, cannot supply the parameters, errors -- and the validator published "the PR adds this test target and it fails on head" against an author whose target is green as a script with the very shapes the run had requested (verified by hand: -m 97 513 -n 512, all checkAllclose passed). A test* function is now only counted as a pytest node when it takes no required positional parameters or carries a parametrize/fixture decorator. #5081 selects `script`, the grid and the axis reach it, and no blocker is raised. `--runner` lets a caller settle the case the classifier still gets wrong, recording both the forced choice and what selection had said. A route behind functools.wraps could never be observed. A frame's identity is f_globals["__name__"] + ":" + f_code.co_name, and wraps copies __name__ onto the wrapper object while leaving co_name alone -- so aiter's whole @compile_ops family executes as `aiter.jit.core:wrapper` and naming the op matched nothing. The probe now resolves the declared route to code objects through the __wrapped__ chain; the string match remains as a fallback for a module that cannot be imported. On #5081 the receipt goes from observed '' to a real observed route, and INCONCLUSIVE now rests on the honest remaining gap (a (*args, **kwargs) dispatch wrapper binds no shape locals, so the receipt reports missing shapes rather than passing). Two fields answered the same question differently. When a proven axis rescues a duplicate shape grid, the override reached stages.correctness_s1_grid.independence but not test_selection, which had been written earlier -- so one report said "duplicates-target-defaults" and "adds-coverage" at once. Both are now written from the same decision. A control-gated perf stage still published the numbers it had rejected. `status: skip` beside a median_ratio and a regressed_rows list reads as a regression that was merely not acted on. When the control column moves outside PERF_CONTROL_TOL the ratio fields are dropped. Verified on #5081 end to end: novel grid -> grid_independence adds-coverage with axis `-n` proven (target_defaults 384/768/1024/4224, novel 512/1536); duplicate grid + no axis -> the stage downgrades to skip and both fields agree; duplicate grid + proven axis -> rescued, and they still agree. Seventeen new regression tests, all verified to fail against a8d46dcb0; the suite is 68 and green. --------- Co-authored-by: Jin Pan <jin.pan@amd.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: zhiding512 <zhiding512@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.
Motivation
This PR introduces a FlyDSL kernel for computing DeepSeek-V4 sparse attention lightning indexer on
gfx950. The new kernel is drop-in replacement for the existing Triton/Gluon kernel. For the DSv4 prefill shapes it generally faster than the existing Triton/Gluon kernel. Thegfx942version was introduced in an earlier PR #3913.Technical Details
There are two alternative implementation on
gfx950gfx942: Data is streamed directly from DRAM to registers.The LDS staged instance can have two or more LDS buffer combined with one or more prefetch stages. Allocation of one LDS buffer more than the prefetch depth (
PD) enables to reduce the number of required barriers by one (barriers are required to ensure data is ready before running MFMA on it). In practice, the most performant combination is 3 LDS buffers withPD=2(only one barrier required).There is instance auto-selection which selects the best kernel for a given shape. However, similarly to the existing implementation, the auto-selected instance can be overridden with an environment variable.
There's also one common optimization for both
gfx942andgfx950: whenclean_logits=True, the baseline implementation usestorch.fillto initialize the logits matrix with-infvalues. Since half of the values are subsequently overwritten by the logits kernel, we have fused the "clean logits" initialization step into the logits kernel. This provides performance boost for small sequence lengths.Test Plan
All tests added in PR #3913 are now enabled for
gfx950. For performance, benchmarked the same DSv4 shapes in PR #3913Test Result
All unit tests pass on both
gfx942andgfx950(note: they are not enabled as part of the CI workflow)For
gfx950, the new kernel is compared against the existing Triton/Gluon kernelVerification is done against the existing Triton implementation.
For
gfx942, the changes are compared against the baseline FlyDSL kernelFor all cases the number of heads (H) is 64. Verification against the existing Triton kernel passes.
A recent PR #4563 improved the Gluon kernel for the GLM -5 shapes where
H=32. Here's a comparison of this FlyDSL results against the latest Gluon logits kernel the shapes are taken from PR #4563Submission Checklist