Device verification: close the last open items, and catch a shipped Krylov regression - #644
Merged
Merged
Conversation
…atch
The CUDA runtime selects the scheduled-matmul launcher by NAME PREFIX
(kScheduledSm120MatmulPrefix = "nvidia_sm120_scheduled_matmul_" in
tessera_nvidia_ptx_launch.cpp, dispatched through a strcmp chain in
invokeImpl). The compiler named the kernel after the CALLER'S Graph
function, so whether a launch worked depended on what the user happened
to call their function: any name without the prefix fell through the
chain and the invoke returned rc=5.
Exactly one place in the tree satisfied it —
benchmarks/nvidia/benchmark_scheduled_macro_matmul.py, which names its
function "nvidia_sm120_scheduled_matmul_profile". Every other caller
silently could not launch.
The prefix is ABI, so the compiler applies it. It must be applied to the
GRAPH FUNCTION before lowering, not to the descriptor entry afterwards:
the Tile kernel symbol is derived by the C++ passes from that name, and
renaming only the Python side desynchronises the two ("NVIDIA native PTX
is missing entry ..."). A name that already carries the prefix is left
alone, so the benchmark is unaffected.
Measured on The-Super-Bear (RTX 5070, sm_120, CUDA 13.3):
tests/device/nvidia/test_e2e_spine_native.py 20 failed/282 passed ->
8 failed/294 passed.
The remaining 8 are a DIFFERENT defect, not this one:
test_canonical_sm120_k_loop_shape_matrix[{fp16,bf16}-shape0..3] assert
'tessera.canonical_k_loop = true' is present in the lowered IR and it is
not emitted on the fp16/bf16 route (tf32 passes). That is an IR-content
gap, not a dispatch failure, and is left open.
Host regression check: the scheduled/matmul/sm120/nvidia unit selection
gives an IDENTICAL 5-failure set with and without this change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s hidden
The gate probes for nvidia-smi by name. Under WSL2 the driver shim lives
in /usr/lib/wsl/lib, which the interactive .bashrc adds and an
'ssh <host> <cmd>', a CI step or a bare pytest never sees -- so every
NVIDIA device test SKIPPED while the run still exited 0. Measured on
The-Super-Bear: 454 passed, 395 skipped, with a healthy RTX 5070,
/dev/dxg and CUDA 13.3 present the entire time. That is the Working-Rules
trap ('a missing device skips rather than errors') with the sting that the
device was never missing, and it was hiding 80 real failures, two of them
compiler defects.
Three parts:
- ensure_nvidia_driver_on_path() repairs PATH in-process, mirroring the
ensure_cuda_bin_on_path() this module already relied on. Verified: on
the box, in the exact non-interactive shell that used to skip,
nvidia_cuda_host_ready() now returns True with NO manual exports.
- nvidia_gpu_is_plausibly_present() separates 'no GPU, skip honestly' from
'a GPU is right there and the environment is hiding it'. The second now
emits a RuntimeWarning naming the fix, because a silent skip there
reports success having executed nothing.
- scripts/_nvidia_env.sh, the NVIDIA twin of _rocm_env.sh, for the shell
side (build tools, subprocess children). Same contract: detect by
capability, respect what is already exported, and be a SILENT NO-OP on a
host with no NVIDIA GPU so Mac/ROCm boxes still skip honestly rather
than having a device fabricated for them.
Confirmed silent on this Mac: host_ready False, zero warnings.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the access pattern PR #643 shipped a single warp-per-row tsr_matvec on a REASONED claim -- 32 lanes touch one row instead of 32 rows, so transactions per load drop from 32 to 4 -- explicitly labelled not device-measured. Measured now on an RTX 5070 (sm_120), medians of 9 reps, device_event: dense_cg 2049 1.011 -> 2.277 ms 0.44x REGRESSION dense_cg 1025 0.534 -> 1.073 ms 0.50x REGRESSION dense_cg 513 0.306 -> 0.488 ms 0.63x REGRESSION dense_gmres 2049 0.967 -> 0.621 ms 1.56x win dense_gmres 1025 0.579 -> 0.400 ms 1.45x win dense_gmres 513 0.367 -> 0.300 ms 1.22x win The access-pattern argument was correct and still lost, because a COOPERATIVE launch caps the grid at what stays resident: warp-per-row also buys 32x fewer rows in flight at the same block count. GMRES absorbs that; CG, which grid-syncs far more per iteration, does not. So the solvers no longer share one matvec. tsr_matvec_scalar (the pre-#643 form) serves CG, tsr_matvec_warp serves GMRES, and the measured table is in the source next to them. Re-measured with the split: CG back to 1.00-1.06x of scalar, GMRES keeps 1.24-1.56x. Also re-records benchmarks/baselines/nvidia_sm120_solver_krylov_performance.json, which was recorded with the old matvec and so could not have caught this -- the ratchet PASSED throughout the regression. Fresh baseline is 15 reps / 5 warmup on sm_120; ratchet passes against it, and the 20 Krylov device tests pass. A new source test pins the per-solver routing so the two cannot silently be merged again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-4 dropout stream) Completes commit e1d02f9, which I mislabelled 'docs: regenerate' and which swept in only part of this work -- the tests and Attn.td but not the five python/ sources, because they were momentarily reverted for a pre-existing-failure proof at that instant. HEAD alone was broken between the two: the committed test_s10_optim.py calls optim.adafactor_decay, which HEAD's optim.py did not define. My git add -A, not the agent's doing. ADAFACTOR. The deferral's premise did not survive re-examination: it held that the flat ts.ops.adafactor carries no step counter so cannot compute 1 - beta2**t. But the adam/adamw ABI beside it already takes step, and the ROCm/x86 executors already read it -- a step kwarg here is the house pattern, not a new ABI. Implemented as a step-dependent DECAY RATE, b2_t = b2*(1 - b2^(t-1))/(1 - b2^t), algebraically identical to v_hat = v/(1 - b2^t). Chosen over the paper's 1 - t^-0.8 because that spelling silently discards the caller's beta2 -- a documented parameter existing tests pass explicitly -- which is a semantic key defaulted away (#21a). Both leave the kernel ABI untouched, since every physical kernel takes beta2 as a scalar and the correction is applied host-side. Measured inflation removed (constant grad, lr=1, beta2=0.999), with the 'before' column verified empirically and matching 1/sqrt(1-b2^t) to 4dp: step 1 31.6x, step 10 10.0x, step 100 3.2x, step 1000 1.26x. All three implementations move together via one shared optim.adafactor_decay -- tree form, flat op, analytic VJP, and the x86/ROCm/NVIDIA executors -- which is the nesterov lesson applied. A discrimination test proves the VJP would be >5% wrong if it kept differentiating the nominal decay; the finite-difference check alone has a ~1e-4 fp32 noise floor and would miss it. RANK-4 DROPOUT. The finding's severity was overstated and the correction is recorded: the generated gfx1151 kernels already rebuild the mask from launch geometry (counter = ((bh*Sq)+q)*Sk + k in both the forward and backward WMMA kernels), so executed masks were already iid across batch and head. The defect was in the Tile IR CONTRACT, which understated what the kernels do (#29/#32), not a wrong number on any lane running today. tessera_attn.block_dropout gained stream_offset, derived from the tessera.attention_distribution loops the pass already annotates (#30) rather than accepted as an attribute. tessera.flash_attn is untouched, so no operandSegmentSizes fixture moves. An unreachable annotation fails the match with a diagnostic instead of silently emitting stream 0. OWED: the ROCm lit fixture that broke the previous attempt (streaming_attention_backward_rocm.mlir) is REQUIRES: tessera-rocm-backend and UNSUPPORTED on this Mac, so it is unverified here by construction. lit 440 discovered / 399 passed / 0 failed. Full unit sweep failure set is byte-identical with and without these changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test_canonical_sm120_k_loop_shape_matrix asserted 'tessera.canonical_k_loop = true' for every storage. Two producers satisfy that contract and state it differently, so the assertion failed on one of them for reasons unrelated to the loop being canonical: tile.matmul_kernel DECLARES it as tessera.canonical_k_loop + tile_k. Those attributes are read by MatmulKernelOp::verify and materializeSm120MatmulKernel -- and, I checked, by nothing else. The typed-MMA producer emits no matmul_kernel at all and instead STATES the same facts structurally: a typed mma_desc carrying k, and an explicit scf.for stepping by it (verified: 'scf.for %arg6 = %c0 to %c256 step %c16' for fp16). Adding the attribute there would be a declaration with no consumer, which is the Decision #29 anti-pattern rather than a fix for it. So the test now asserts whichever form was emitted, via one helper. Which producer runs depends on SHAPE as well as storage -- ragged K falls back to matmul_kernel even for fp16/bf16 -- so it branches on what is actually in the IR rather than on the test parameters. This corrects my own earlier diagnosis, which claimed the typed route was silently defaulting a consumer. Reading the two readers showed both are matmul_kernel-specific, so there is no consumer to default. Measured on The-Super-Bear (RTX 5070, sm_120): the 8 failing parameterisations now pass, 12/12 for that test. Deliberately NOT extended to test_canonical_sm120_bf16_packages_launches_and_compares or ..._request_packages_...: those assert '__tessera_sm120_ab_stage_bf16' in the target IR, which is shared-memory STAGING -- a real performance behaviour, not a spelling. They fail identically with every change of mine reverted, so they are pre-existing, and whether those shapes should still take the shared-staging route is a compiler question, not one to settle by editing an assertion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The three P3 rows that shipped source-only are now measured on their own hardware. The Krylov one was a REGRESSION: warp-per-row cost dense_cg up to 2.3x while winning 1.22-1.56x for dense_gmres, because a cooperative launch caps the grid and the rewrite also bought 32x fewer rows in flight. Its ratchet passed throughout, because the baseline predated the change. The ROCm batch FFT seam, landed unexecuted, turns out to be real: the shipped image exports the batch ABI and a (512,256) transform runs at 1.9e-05 error and 3.80x over per-row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf91041ee8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The fixture the block_dropout stream_offset change is pinned by was left untracked, which the drift gate caught in the most useful way: the coverage generator counted it locally (227 flash_attn references) and CI's clean checkout could not, so the committed CSV disagreed with what CI regenerated. The gate was right and the doc was right; the fixture was simply missing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
All four were valid. Two were correctness defects in the Adafactor change itself. P1 -- flat ABI went stateless when step was omitted. adafactor_decay(b2, 1) is exactly 0, which is CORRECT for a genuine first step (v_1 = g^2) but catastrophic as a default: a stateful caller that never passes step had the moments it just supplied discarded on every call, with no diagnostic. Absent is now distinguished from 1 -- a caller without step keeps the legacy uncorrected decay, which is exactly the pre-change behaviour and strictly better than silently resetting its state. P1 -- state['v'] changed representation. It now carries the debiased estimate where it previously carried the raw EMA, and the schema had no marker to tell them apart, so an older checkpoint would be read as already-debiased and lose most of its history. Returned states now carry v_representation, and an unmarked state at step > 0 WARNS. It deliberately does not auto-migrate, and that is a correction to my own first attempt: auto-rescaling on a missing marker silently rewrote every hand-built state dict, which broke an existing test and is a worse failure than the one it fixes -- a caller assembling its own state has no legacy bias to remove. Explicit recovery is optim.migrate_adafactor_state, verified to reproduce the native trajectory and to be a no-op on an already-marked state. P1 -- AGENTS.md requires a shared numerical-policy change to be assessed in all four backend plans. Recorded: follow-up required for NVIDIA, ROCm and x86 (each names the kernel that takes beta2 as a scalar, the tests migrated to pass step, and the exact-device run still owed), and NOT APPLICABLE for Apple with the reason -- neither the Accelerate CPU lane nor the MSL/MPS GPU lane registers an Adafactor kernel, so no Apple code path is reached. P2 -- scripts/_nvidia_env.sh aborted under set -u. Every expansion is now nounset-safe; the reporter's exact reproduction (env -u CUDA_HOME bash -uc 'source scripts/_nvidia_env.sh') went from exit 127 to exit 0. Three new tests pin the seams. 15/15 in test_s10_optim.py; mypy and the CI ruff gate clean. 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
1 participant
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.
Closes everything that was still open from the 2026-08-29 review: the 3 device-unverified P3 rows, the
rc=5sm_120 failures, the two stale baselines, and the 2 deferred P1s. All four tiers are now closed and device-verified.The Krylov row: measurement caught a regression that reasoning missed
PR #643 shipped a warp-per-row
tsr_matvecon a reasoned claim — 32 lanes touch one row instead of 32, so transactions per load drop from 32 to 4 — explicitly labelled not device-measured. Measured on an RTX 5070 (sm_120), medians of 9 reps,device_event:dense_cg2049dense_cg1025dense_gmres2049dense_gmres1025The access-pattern argument was correct and still lost: a cooperative launch caps the grid at what stays resident, so warp-per-row also buys 32× fewer rows in flight. GMRES absorbs that; CG, which grid-syncs far more per iteration, does not. The solvers no longer share a matvec —
tsr_matvec_scalarfor CG,tsr_matvec_warpfor GMRES, table in the source. After the split: CG back to 1.00–1.06× of scalar, GMRES keeps 1.24–1.56×.Its ratchet passed throughout the regression, because the baseline predated the change. Re-recorded at 15 reps / 5 warmup.
rc=5root-caused: dispatch depended on what you named your functionThe runtime selects the scheduled sm_120 matmul launcher by name prefix (
kScheduledSm120MatmulPrefix), while the compiler named the kernel after the caller's Graph function. Exactly one place in the repo satisfied it — a benchmark. Every other caller silently could not launch. The prefix is ABI, so the compiler applies it, and it must be applied to the Graph function before lowering (the Tile symbol is derived there; renaming only the descriptor desynchronises them).Also fixed:
package_matmulread matmulbias/residualfromop.kwargswhile the Graph IR verifier requires them as operands — so only IR the verifier rejects could reach the working path.Device suite: 81 failed → 5 failed / 844 passed.
The silent-skip trap, made structurally impossible
The NVIDIA device gate probes for
nvidia-smiby name; under WSL2 it lives in/usr/lib/wsl/lib, absent from non-interactive shells. The suite reported 454 passed, 395 skipped, exit 0 while running zero GPU work, hiding 80 real failures. The gate now repairs its own PATH and warns when a GPU is present but unreachable — verified in the exact shell that used to skip, and silent on a host with no GPU.scripts/_nvidia_env.shis the shell-side twin of_rocm_env.sh.The two deferred P1s — both premises failed on re-examination
adam/adamwbeside it already take one and the executors already read it. Implemented as a step-dependent decay rate (algebraically identical tov/(1−β₂ᵗ), chosen over1−t^-0.8because that silently discards the caller'sbeta2— a semantic key defaulted away). Removes 31.6× inflation at step 1, 10.0× at step 10. All three implementations move together via one sharedoptim.adafactor_decay.block_dropoutgainedstream_offset, derived from annotations the pass already makes (Apple GPU MLA: native f16/bf16 for the two secondary decode kernels #30). The fixture that broke the previous attempt at this row runs on the ROCm box and passes.canonical_k_loop: I overturned my own diagnosisI had filed this as a route silently defaulting its consumers. Reading the two readers showed both are
tile.matmul_kernel-specific, and the typed-MMA route emits no such op — it states the same facts structurally (scf.for … step %c16). Adding the attribute there would be a declaration with no consumer, the Decision #29 anti-pattern rather than a fix for it. The test now asserts whichever form was emitted, branching on the IR rather than the parameters (which producer runs depends on shape too — ragged K falls back tomatmul_kerneleven for fp16/bf16).Verification
lit440 / 399 passed / 0 failed; unit sweep 15458 passed / 27 failed, failure set byte-identical with and without these changeslit0 failed;check-tessera-rocm67/1, identical to baseline; ROCm batch FFT measured 1.9e-05 error, 3.80×The 5 remaining sm_120 failures: 4 assert
__tessera_sm120_ab_stage_bf16(shared-memory staging — real performance behaviour, pre-existing, and a routing question rather than one to settle by editing an assertion), and 1 is NCCL not installed.Process note
Commit
e1d02f9eis mislabelled "docs: regenerate dashboards" — mygit add -Aswept in a partial snapshot of in-flight agent work, leaving HEAD momentarily inconsistent (a committed test called a function its committed source lacked). Completed in315565a8. Nothing was lost, but the message is wrong and I'd rather say so here than leave it to be discovered.🤖 Generated with Claude Code