Repository navigation
[Bugfix][Kernel] Fix fp8 dense attention: softmax normalisation and an int32 dim overflow - #1020
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes two independent correctness issues in dense fp8 flash-attention: (1) softmax normalization error caused by fp8 underflow in the PV path, and (2) a launch-time crash from int32 shape packing overflow when flattening very large tensors. The changes are localized to fp8 dense attention utilities, the Python interface wrapper, and the gfx950 fp8 kernel path.
Changes:
- Add an optional log2-domain bias to the fused
scale*(score-row_max)helper to lift softmax probabilities before fp8 casting (avoiding tail flush-to-zero mass loss under fp8 e4m3). - Add a dense-fp8 guard to split very-large
B*S*H*Dlaunches into per-batch-entry launches to avoidstruct.packint32 shape overflow. - Honor
DUALWAVE_SWP_LAZY_RESCALEby routing through a helper that selects lazy vs eager rescale correction.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| kernels/attention/flash_attn_utils.py | Adds optional bias support in softmax pre-processing to prevent fp8 PV underflow from breaking normalization. |
| kernels/attention/flash_attn_interface.py | Adds a size-triggered per-batch split path for dense fp8 to avoid int32 ABI shape packing overflow. |
| kernels/attention/flash_attn_fp8_gfx950.py | Ensures the lazy-rescale flag is actually respected by selecting the appropriate online-rescale correction path. |
Suppressed comments (1)
kernels/attention/flash_attn_utils.py:434
- The
_scale_sub_score_pairdocstring still describes the return asscale * (v_s - row_max_raw), but the function now also supports an optionalbiasterm (log2-domain shift). Updating the docstring will prevent future callers from missing the bias behavior when reading this helper.
Returns ``scale * (v_s - row_max_raw)`` per element via a single FMA
(``fma(s, scale, -scale*row_max_raw)``), so the fp8 QK MMA can emit raw
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
0914e81 to
816ebd8
Compare
The fp8 helper has the same wave-uniform branch as the bf16 one, and the same
two lanes-dragged-along cases, but it cannot take the max: rebasing to a lower
running max is what lets it keep P large against the base, and pinning the base
costs accuracy at ranges that work today -- mean |o-1| goes 0.0001 to 0.0088 at
k*1000 with V=ones.
What overflows is not the rebase but its lack of a bound. corr = exp2(-m_diff_scaled)
grows without limit as the score range widens, so cap the drop instead: rebase
while the correction stays under 2**16, and leave the lane alone past that.
The threshold is separate from DUALWAVE_SWP_RESCALE_THRESHOLD, which decides
when to rescale rather than how far down. Measured at S=12288, V=ones, mean
|o-1| and NaN count:
k scale before bound 8 bound 16 bound 32
1000 0.0001, 0 NaN 0.0021 0.0001 0.0001
5000 0.0000, 20.0M NaN 0.0025 0.0019 0.0001, 887k NaN
20000 nan, 25.2M NaN -- 0.0006 0.0006, 2.4k NaN
16 keeps every working range bit-for-bit where it is and takes the NaN to zero;
32 starts leaking again. Verified both on main and on top of #1020.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4482b31 to
fc1c5a7
Compare
The fp8 helper has the same wave-uniform branch as the bf16 one, and the same
two lanes-dragged-along cases, but it cannot take the max: rebasing to a lower
running max is what lets it keep P large against the base, and pinning the base
costs accuracy at ranges that work today -- mean |o-1| goes 0.0001 to 0.0088 at
k*1000 with V=ones.
What overflows is not the rebase but its lack of a bound. corr = exp2(-m_diff_scaled)
grows without limit as the score range widens, so cap the drop instead: rebase
while the correction stays under 2**16, and leave the lane alone past that.
The threshold is separate from DUALWAVE_SWP_RESCALE_THRESHOLD, which decides
when to rescale rather than how far down. Measured at S=12288, V=ones, mean
|o-1| and NaN count:
k scale before bound 8 bound 16 bound 32
1000 0.0001, 0 NaN 0.0021 0.0001 0.0001
5000 0.0000, 20.0M NaN 0.0025 0.0019 0.0001, 887k NaN
20000 nan, 25.2M NaN -- 0.0006 0.0006, 2.4k NaN
16 keeps every working range bit-for-bit where it is and takes the NaN to zero;
32 starts leaking again. Verified both on main and on top of #1020.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c299d53 to
e2313b6
Compare
jhinpan
left a comment
There was a problem hiding this comment.
Requesting changes for two correctness regressions in the new fp8 batch-split path.
The softmax headroom change and the signed-int32 boundary hold up in local validation, and the exact head passes the full attention file (106/106). The split path, however, (1) silently discards unsupported bias / alibi_slopes / sink arguments before validation and (2) can copy side-stream outputs on the current stream before the kernels finish when out=None. Both were independently reproduced; the inline comments include minimal fixes and regression-test specifications.
One smaller durability item: the library's new fp8 default is eager, but test_fp8_softmax_normalises always passes the flag explicitly and the CLI/benchmark configuration still forces dualwave_swp_lazy_rescale=True. Please let the CLI inherit None and add one test that omits the keyword and matches explicit eager. This is secondary to the two blockers above.
jhinpan
left a comment
There was a problem hiding this comment.
The two split-path correctness blockers are fixed. I re-ran the original modifier and delayed-side-stream reproductions, the new targeted cases, and the full attention file: 17/17 targeted and 110/110 overall passed on gfx950.
One requested item remains: the PR changes fp8's library default to eager, but the first-party CLI/benchmark path still forces the old lazy value, and no test can detect a default regression. See the inline comment. Requesting changes only for that remaining default/coverage inconsistency; the implementation fixes themselves hold up.
jhinpan
left a comment
There was a problem hiding this comment.
LGTM. The split-path modifier validation and explicit-stream fix remain correct, and the final default/CLI gap is now closed: the config and forwarding sites carry None, the library resolves per dtype, both CLI overrides remain available, and the omitted-keyword fp8 test matches explicit eager bit for bit.
Exact-head gfx950 validation: 13/13 focused cases and the full attention file 111/111 passed. GPU CI jobs are still pending at review time; the completed style, docs, signal, and Navi checks are green.
6de2a37 to
ef65bdb
Compare
jhinpan
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ef65bdb after the rebase. The eight-commit patch matches the previously approved series; the modifier validation, side-stream output fix, and CLI/default coverage remain intact. On a checkout-matched dd83733 runtime, all 18 focused fp8 regressions and the full attention file (116/116) pass on an isolated gfx950. The current check-signal failure is its pre-check polling timeout, not a code-test failure.
4f81e71 to
637e448
Compare
|
LGTM |
With V set to all ones the output must be exactly 1.0 everywhere, since softmax
weights sum to 1. It came back as low as 0.637, with 58-83% of rows below 1.0.
bf16 on the same inputs deviates by at most 4e-05.
P is cast to e4m3 for the PV MMA and e4m3's smallest subnormal is 2**-9. A
softmax over thousands of keys puts most of the tail below that, where it
flushes to zero -- while l_row, summed before the cast, still counts it. The
output is divided by a sum that includes weights the matmul dropped. It only
shows on peaked attention, so a random-init model does not reveal it.
Scale P up before the cast so it uses e4m3's whole range rather than its top
octave. l_row scales with it, leaving the output unchanged apart from the tail
that no longer underflows, and the shift rides the addend of an FMA that already
runs, so it is free.
How much scale is safe depends on how large exp2 can get: `lazy_correct_o` holds
the running max until a tile exceeds it by RESCALE_THRESHOLD log2 units, so
exp2 <= 2**THRESHOLD there and log2(448) - THRESHOLD is left over, while the
eager path rebases every tile and all of log2(448) is available. The scale is
derived from that and applies on both paths.
At the default threshold of 8 that is a 2x scale, worst row 0.637 -> 0.766, at
unchanged speed. Lowering the threshold buys the rest, measured at B=2 S=12288
H=8 D=128 with a wide score range:
threshold P scale worst row TFLOP/s
8 (default) 2x 0.766 1641
6 7x 0.910 1632
4 28x 0.988 1548
2 112x 0.996 1498
0 (eager) 448x 0.996 1506
There is no cheap middle ground: by the time the threshold is low enough to fix
the row error it costs what the eager path costs. The default is left alone.
Doing this surfaced a second defect. Both call sites invoked `lazy_correct_o`
unconditionally, so DUALWAVE_SWP_LAZY_RESCALE reached the traits and was never
read and setting it did nothing. They now go through a helper that honours it.
The test uses the V=ones property, so it needs no reference implementation, and
covers both rescale paths at the bound each can reach.
A dense fp8 launch raises once B*S*H*D reaches 2**31:
struct.error: 'i' format requires -2147483648 <= number <= 2147483647
At D=128, H=64 that is S >= 131072.
jit_argument._LayoutPlan packs shapes as int32 -- "i" * len(shape), with only
the strides following use_32bit_stride -- and the fp8 path flattens Q/K/V/O to
1-D, so the packed dim is the whole element count. bf16 passes the natural 4-D
shape, where no single dim is that large; the comment above that branch already
warns about this.
Batch entries are independent in attention and a leading slice of a contiguous
[B,S,H,D] tensor is itself contiguous, so one launch per entry divides the flat
dim by B at no copy and no kernel change. Guarded on numel, so everything below
the bound takes the path it takes today. Splitting costs 3.2% at S=27k, 0.9% at
S=61k and nothing by S=121k, and only runs past S=131072.
Two alternatives that do not work, so they are not worth retrying: passing the
natural 4-D shape for fp8 returns garbage at sizes that work today (rel 1.7e+05
at S=61380 against a correct 2.4e-02) because the fp8 module addresses from the
flat descriptor; and passing another flat-equivalent view such as [B, -1] clears
the error and produces silently wrong output, which is worse than the current
loud failure.
A caller-supplied `out` is returned as given. Each per-entry call writes into its
own out[i:i+1] view, so concatenating would copy several GB again at the sizes
that reach this branch and hand back a different tensor than was passed in.
Reaching the branch for real costs ~10 GB of q/k/v/out, so the bound is a named
constant and the test lowers it to cover the split on a small tensor -- against
the unsplit result, since splitting must not change the output.
The lazy path cannot lift P far enough to keep the softmax tail out of e4m3's
flush-to-zero range, so leaving it on means shipping a known-wrong default. Take
the accuracy.
Measured at B=2 H=8 D=128, V=ones, where the exact output is 1.0:
S k scale worst row mean |o-1| TFLOP/s
1024 1 0.9844 -> 0.9844 0.0139 -> 0.0105 165.0 -> 158.4
1024 200 0.9492 -> 0.9883 0.0034 -> 0.0014 162.7 -> 160.4
12288 1 0.9805 -> 0.9805 0.0137 -> 0.0044 1733.2 -> 1632.1
12288 200 0.7656 -> 0.9922 0.0021 -> 0.0004 1732.0 -> 1611.4
The peaked case is the one that mattered: 0.766 to 0.992. Error drops 3-5x
across the board and costs 1.4-7.0%, most of it at long sequences.
The switch is per dtype rather than global: dualwave_swp_lazy_rescale becomes
Optional and resolves to False for fp8, True for bf16, so bf16 keeps the lazy
path and callers passing the flag explicitly are unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review catch: the guard read q.numel() only and only ran for B > 1, so three cases still reached the C ABI and aborted there — B=1 over the limit, B>1 where one entry is still over after the split, and cross-attention holding a short Q against an over-long K/V. Take the max over Q, K and V, and raise NotImplementedError naming the counts and the limit when there is no batch left to divide. struct.error from inside the packing layer names neither. Three cases added to the tests, all lowering the bound rather than allocating 2**31 elements. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two problems in the flat-dim split, both found in review. The split ran before the fp8 modifier check and its kwargs carried neither bias, alibi_slopes nor sink, so a call large enough to split returned plain attention while the same call one element smaller raised NotImplementedError. The check is hoisted above the split; an oversized B == 1 call now reports the unsupported modifier rather than the int32 limit. With out=None the launches were queued on `stream` and torch.cat then consumed them on the ambient stream with nothing ordering the two. Allocate the output once and give each launch its own slice instead: no cross-stream consumer, and output-side peak drops from two full tensors to one, which matters at the sizes that reach this path. Four regressions: the three modifiers, and a split run on a deliberately busy side stream compared bit-for-bit against the unsplit result. The stream test fails on the previous implementation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The config, the fp8 launcher and argparse each set lazy_rescale True, so --dtype fp8 and the benchmark path kept running the mode this PR moves off by default, and every test passed the flag explicitly so the default itself was never exercised. They carry None now and the library resolves per dtype. The new V=ones case omits the keyword and must match an explicit False; it fails if the library default is flipped back.
--no-lazy-rescale is store_false, so with the default at None the flag could only be turned off -- including for the fp8 lazy/eager comparison this PR's table reports.
Supersedes 2ebf4bf, 6e9d5fe and ef65bdb: fp8 goes back to the lazy rescale that bf16 uses, and its RESCALE_THRESHOLD drops from 8 to 6 instead. Review asked for the accuracy those commits bought without the throughput they cost. sub_m can lift P by log2(448) - RESCALE_THRESHOLD, since exp2 on the lazy path is free to reach 2**THRESHOLD. At 8 that leaves 0.8, too little to keep the tail out of e4m3's flush-to-zero; at 6 it leaves 2.8. The eager path rebases every tile and gets all 8.8, which is why it was the first answer. Measured on eight attention layers whose q and k come from trained projections rather than random init -- random init hides this bug, which needs peaked attention to show. Score spread (row max - mean, in log2) runs 4.3 to 15.2 across the eight; S=4096, V=ones so every row's exact value is 1.0. Worst layer of the eight, throughput as a geometric mean over all of them: threshold 8 worst row 0.867 mae 0.0072 100% threshold 6 worst row 0.957 mae 0.0044 98.2% threshold 4 worst row 0.957 mae 0.0030 95.7% eager worst row 0.981 mae 0.0015 94.0% 6 is the knee -- 4 costs another 2.5% and does not move the worst row at all. The value is not fitted to these layers: the tail survives to 9 + (8.807 - T) below the row max, so 6 buys two log2 units over 8, and it holds the widest layer (spread 15.2) to the same 0.965 as the middling ones. Also here, all consequences of dropping the per-dtype default: - the public signature, its docstring, FLASH_ATTN_FUNC_KERNEL_CONFIG and the benchmark CLI go back to their pre-PR state, since there is no longer a default to resolve and --lazy-rescale would be a no-op - test_fp8_softmax_normalises tightens its lazy bound from 0.30 to 0.12, which the old threshold fails at 0.234, so putting the constant back turns the suite red - the fp8 default test now pins lazy rather than eager - c_eight_f becomes c_rescale_thr_f in the fp8 context, where it now holds 6 - the return_lse/paged_kv check goes back above the modifier validation, where it sat before the split block was added bf16 keeps 8 and is untouched: its sub_m never passes a bias, and the fp8 factory is the only constant that moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sub_m lifts P by log2(448) - RESCALE_THRESHOLD, and the lazy path holds one
running max across every tile, so a longer sequence leaves more tiles far below
it with their tails flushed. A lower threshold lifts P further but fires the
rescale branch more often -- and that cost falls as S grows while the benefit
rises, so the crossover is a length. 6 -> 4 sits just above 4096.
Worst row of eight layers built from trained q/k projections, against main:
thr 6 thr 4
4,096 0.957 -2.7% 0.957 -4.6%
131,072 0.789 -2.1% 0.906 -2.4%
131,072 is Bug 2's own regime, so it is where the default has to be right.
Eager holds 0.98 everywhere but costs 10% there and climbs with S; it stays
reachable through dualwave_swp_lazy_rescale=False.
The kernel is not specialised on S, so this widens the fp8 build cache to two
variants; the bound is a constant to keep it at two. test_fp8_softmax_normalises
tightens its lazy bound to 0.06, which a threshold pinned at 6 fails at 0.09.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing reaches the JIT cache key from a frozen-dataclass traits object except through traits.cache_tag, and DUALWAVE_SWP_RESCALE_THRESHOLD was missing from it. Harmless while the threshold was a constant; once it varies by KV length the S=12288 build loads the S=1024 binary. Only shows with the disk cache on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0ad16ff to
a84b831
Compare
Summary
Two independent bugs in the dense fp8 attention forward on gfx950, one commit each.
struct.errorfor large inputs. Any dense fp8 call withB*S*H*D >= 2**31fails. AtD=128, H=64that isS >= 131072.Bug 1: rows lose softmax mass
Set
Vto all ones and the output must be exactly1.0, since softmax weights sum to 1. bf16 gives 0.99996; fp8 gives 0.637, with 58–83% of rows short.Pis cast to e4m3 before the PV matmul, and e4m3 bottoms out at2**-9. With thousands of keys most weights fall below that and become zero — butl_row, the normaliser, is summed before the cast and still counts them. Only peaked attention shows this; a random-init model will not.Fix: scale
Pup before the cast so it uses e4m3's whole range.l_rowscales with it, so nothing changes but the tail that no longer underflows. Free — the scale rides an FMA that already runs, as an optionalbiason_scale_sub_score_pair; bf16 never passes one.The safe lift is
log2(448) - RESCALE_THRESHOLD, so fp8 picks its threshold from the KV length: 6 at or belowS=4096, 4 above — see below. bf16 keeps 8.This also surfaced that the fp8 kernel never read
DUALWAVE_SWP_LAZY_RESCALE— both call sites usedlazy_correct_ounconditionally, so the flag did nothing. It works now.Bug 2: int32 overflow on the shape
jit_argument._LayoutPlanpacks shapes as int32; only strides honouruse_32bit_stride. The fp8 path flattens Q/K/V/O to 1-D, so the packed dim is the total element count and it overflows. bf16 passes the natural 4-D shape and is exempt.Fix: one launch per batch entry when the total would overflow. Entries are independent and a leading slice of a contiguous tensor is still contiguous, so no copy and no kernel change.
Performance and accuracy (MI355X, gfx950)
Eight attention layers built from trained
q/kprojections — random init hides this bug.V=ones, so the worst row is the least ofmean(o)over rows. Worst layer of the eight; throughput relative to main. Bold is what this PR selects.Scaling
Pis free; the threshold is what costs. AtS=4096the scaling alone is −1.2% and takes the worst row 0.867 → 0.910; the threshold takes it the rest of the way.Why not one threshold. The lazy path holds one running max across every tile, so as
Sgrows more tiles sit far below it and lose their tail — 6 falls 0.957 → 0.789 across this range while 4 holds 0.906–0.965. But at and below 4096 the two are equally accurate and 6 is 2 points cheaper. Both curves move withSin opposite directions, so the crossover is a length.S=131,072is Bug 2's own regime — the second commit exists to let that shape launch at all — which is where the default has to be right.Why not eager. It is flat at 0.98, but its cost climbs with
S(−1.7% → −10.2%) while threshold 4's falls, so it is worst exactly where help is needed most. It stays reachable asflydsl_flash_attn_func(..., dualwave_swp_lazy_rescale=False), which now selects it on fp8 instead of being ignored.The kernel is not specialised on
S, so selecting on it widens the fp8 build cache from one variant to two; the bound is a constant to keep it at two. bf16 is untouched by construction and measured unchanged.Testing
test_fp8_softmax_normalisesneeds no reference implementation, and its lazy bound pins the per-length choice: pinned at 6 it reaches 0.09 and at 8 it reaches 0.23, both past the 0.06 allowedtest_flash_attn_fwd.pypasses — 117 passed cold (FLYDSL_RUNTIME_ENABLE_CACHE=0)Breaking Changes
None.