PB-4: compose the TSOL spectral family over Tessera's own FFT; fix a silent wrong answer at non-power-of-two sizes - #495
Merged
Conversation
…t wrong answer
Tessera already ships its own FFT -- Stockham radix-4 at
`src/solvers/spectral/lib/TargetHooks/{CPU,AMD,NVIDIA}/StockhamRadix4.*` -- with
the CPU and AMD lanes registered as arbiter candidates. The other five TSOL
spectral ops had no region and no candidate on any target, so nothing
arbitrated for them anywhere.
They are not new kernels. Each decomposes to the complex FFT the shipped lanes
already implement, so a candidate here delegates its inner transform. One FFT
lane per foundation now lights up five TSOL ops on that foundation, and
registration is driven off the FFT registry rather than a hard-coded target
list -- so the NVIDIA `.cu` (written, never registered) and a future Apple lane
bring the composed ops with them automatically.
Verified on this box, CPU and live gfx1151, against numpy:
rfft cpu 1.12e-07 rocm 2.25e-07
irfft cpu 1.39e-07 rocm 1.86e-07
stft cpu 1.67e-07 rocm 1.68e-07
istft cpu 3.34e-06 rocm 1.65e-06
spectral_filter cpu 0.00e+00 rocm 0.00e+00
`spectral_filter` is deliberately NOT composed over the FFT: it is a pointwise
product of two spectra with no transform inside. Filing it with the transforms
because its operands happen to be spectra would repeat the one-reason-for-N-ops
error that hid `kv_cache.read` among the cache mutators.
THE IMPORTANT FIND, and it is pre-existing: the shipped Stockham kernels are
power-of-two only, do not validate `n`, and DID NOT DECLINE. Measured against
numpy on both lanes -- every power of two from 1 to 1024 agrees to ~1e-7, while
3, 12, 24, 48, 100, 255 and 257 come back with relative error ~1.0. A different
answer, not a precision shortfall. Both kernels returned their own lane name,
so the arbiter recorded a successful `cpu_stockham` / `rocm_stockham` run for a
wrong transform. Decision #21 requires an unsupported case to say so; a
plausible-looking wrong array is worse than the silent no-op it prohibits.
The F4 verifier never caught it because it only ever ran at power-of-two sizes.
Fourth time in this registry work that a gate was only as good as the
configuration it measured -- after `ebm_self_verify` (same-shaped operands), the
collectives (world_size=1), and `nonzero` (a stacked tuple).
Fixed at the candidate layer via `applies_to`, which the arbiter already
consults: unsupported sizes decline to the reference, which is correct rather
than merely absent. The composed ops inherit the restriction on the length
ACTUALLY transformed -- `stft`/`istft` transform a WINDOW, so a 1000-sample
signal with a 64-sample window is fine while a 1024-sample signal with a
60-sample window is not; reading the signal length would be wrong both ways.
Extending the kernels to arbitrary `n` (mixed-radix, or Bluestein for primes)
is real work and separate. Declining is what makes the current kernels honest
meanwhile.
TSOL `backend_kernel` stays `partial` for the spectral family and that is the
accurate reading: two of four foundations are proven, not four.
Unit 14304 passed / 0 failed; mypy 0; ruff clean; 24 generated docs in sync.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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: e7332e49f2
ℹ️ 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".
Comment on lines
+360
to
+361
| def window(self) -> np.ndarray: | ||
| return np.hanning(self.win).astype(np.float32) |
There was a problem hiding this comment.
The public operations are stft(x, win, ...) and istft(xf, win, ...), but these regions manufacture a Hann window while the candidate run methods consume only x/xf and silently ignore additional operands through *a. Consequently, any rectangular or custom window is computed as Hann while both the verifier and fallback compare against the same manufactured window, allowing a silently wrong result to pass verification; the supplied window needs to participate in the region reference, probe, and candidate execution.
Useful? React with 👍 / 👎.
| """ | ||
| from tessera.compiler.emit.candidate import _CANDIDATES | ||
|
|
||
| targets = {t for (t, op) in _CANDIDATES if op == OP_SPECTRAL_FFT} |
There was a problem hiding this comment.
This snapshots _CANDIDATES only once during module import, so an FFT candidate registered afterward by the plugin-style registry receives none of the promised RFFT/IRFFT/STFT/ISTFT/filter candidates. This affects the stated NVIDIA/Apple extension path whenever their module is imported after spectral_candidates; composition registration needs to be triggered by later FFT registrations or performed lazily rather than from this one-time snapshot.
Useful? React with 👍 / 👎.
gstoner
changed the title
… compose P1 -- the caller's window was silently replaced by Hann. `SpectralSTFTRegion` and `SpectralISTFTRegion` manufactured `np.hanning(win)` internally and the candidates ignored the operand through `*a`. A rectangular or custom window was therefore computed as Hann -- and because the REFERENCE manufactured the same window, verification compared wrong against wrong and passed. That is the same failure this thread keeps rediscovering: a gate that only measures a configuration where a right and a wrong implementation agree. Third instance in this file's own history, after the FFT verifier probing only powers of two and the composed-lane count sharing a file with two other products. The window is now an operand on the reference, the probe and every candidate. The probe window is deliberately NOT Hann -- asymmetric, not a standard taper -- so a lane that invents its own fails immediately, and the gate asserts up front that the two windows give different spectra, because otherwise it proves nothing. The verifier is also unified into one `_verify_region` that reads operand arity from the region's own `probe_input`. The arity had been written down separately from the region, which is exactly how the window operand went unverified. P2 -- composition was built from a one-time registry snapshot. `spectral_fft` candidates registered AFTER this module imported received none of the composed ops, silently invalidating the advertised NVIDIA/Apple extension path (those modules may well import later; registration order between backends is not controlled). The PR claim that "one FFT lane per foundation lights up five TSOL ops" was true only for lanes that happened to import first. `register_candidate` now fires a registration hook, and the spectral module subscribes: a target gains its composed ops the moment it gains an FFT lane, whenever that is. The hook list is a general registry facility rather than a spectral special case, since any derived-candidate family has the same problem. A hook that raises cannot break the primary registration -- the base lane matters more than the compositions built on it. Verified: late-registered lane gains all five composed ops; custom-window STFT and ISTFT match the reference on CPU and gfx1151 (1.07e-07 / 2.23e-07) while differing from the Hann answer. Unit 14307 passed / 0 failed; mypy 0; ruff clean. Co-Authored-By: Claude Opus 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.
Extends the TSOL spectral family over Tessera's own FFT — no vendor libraries. Verified on CPU and live gfx1151.
What was already there
Tessera ships Stockham radix-4 at
src/solvers/spectral/lib/TargetHooks/{CPU,AMD,NVIDIA}/StockhamRadix4.*, with the CPU and AMD lanes registered as arbiter candidates forspectral_fft. The other five TSOL spectral ops had no region and no candidate on any target, so nothing arbitrated for them anywhere.Composition, not new kernels
rfft/irfft/stft/istfteach decompose to the complex FFT the shipped lanes already implement, so a candidate delegates its inner transform. One FFT lane per foundation now lights up five TSOL ops on that foundation, and registration is driven off the FFT registry rather than a hard-coded target list — so the NVIDIA.cu(written, never registered) and a future Apple lane bring the composed ops with them automatically.Measured against numpy, both foundations:
rfftirfftstftistftspectral_filterLane provenance is reported through the chain —
rocm_stockham+irfft+istft— so a composition that silently fell back to the reference is distinguishable from one that ran the kernel.spectral_filteris deliberately not composed over the FFT: it's a pointwise product of two spectra with no transform inside. Filing it with the transforms because its operands happen to be spectra would repeat the one-reason-for-N-ops error that hidkv_cache.readamong the cache mutators.The important find — pre-existing, and a silent wrong answer
The shipped Stockham kernels are power-of-two only, do not validate
n, and did not decline.A different answer, not a precision shortfall. Both kernels returned their own lane name, so the arbiter recorded a successful
cpu_stockham/rocm_stockhamrun for a wrong transform. Decision #21 requires an unsupported case to say so; a plausible-looking wrong array is worse than the silent no-op it prohibits.The F4 verifier never caught it because it only ever ran at power-of-two sizes. That's the fourth time in this registry work that a gate was only as good as the configuration it measured — after
ebm_self_verify(two same-shaped operands), the collectives (world_size=1), andnonzero(a stacked tuple).Fixed at the candidate layer via
applies_to, which the arbiter already consults: unsupported sizes decline to the reference, which is correct rather than merely absent. The composed ops inherit the restriction on the length actually transformed —stft/istfttransform a window, so a 1000-sample signal with a 64-sample window is fine while a 1024-sample signal with a 60-sample window is not; reading the signal length would be wrong in both directions.Extending the kernels to arbitrary
n(mixed-radix, or Bluestein for primes) is real work and separate. Declining is what makes the current kernels honest meanwhile.Status
TSOL
backend_kernelstayspartialfor the spectral family, and that is the accurate reading: two of four foundations are proven, not four. NVIDIA's kernel exists but isn't registered; Apple has none.Unit 14304 passed / 0 failed · mypy 0 · ruff clean · 24 generated docs in sync
🤖 Generated with Claude Code