Skip to content

arm64: honor f16 unicast operand in mmm_f32 32x1/32x3 add_unicast - #2495

Merged
kali merged 3 commits into
sonos:mainfrom
czoli1976:fix/arm64-add-unicast-f16
Jul 20, 2026
Merged

arm64: honor f16 unicast operand in mmm_f32 32x1/32x3 add_unicast#2495
kali merged 3 commits into
sonos:mainfrom
czoli1976:fix/arm64-add-unicast-f16

Conversation

@czoli1976

@czoli1976 czoli1976 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

The arm64simd mmm_f32_32x1 / mmm_f32_32x3 and x86_64 fma_mmm_f32_32x1 / fma_mmm_f32_32x3 kernels declare store(f16) but their .add_unicast path never handled an f16 operand: it ignored item_size and always loaded the unicast operand as f32, so a fused f16 AddUnicast (e.g. a residual Add folded into a native q40 GEMV) was read at the wrong width and could saturate to inf.

Each kernel gains the missing item_size == 2 branch that converts the operand before the add — fcvtl on arm64, vcvtph2ps on x86_64 — mirroring the existing store_f16 path (both the contiguous and strided variants).

Tests:

add_unicast_dt (store-dtype-gated): operand dtype = the kernel's declared store dtype, covering the f16-into-f32 case for every f16-storing kernel. Fails on all four kernels without the fix, passes with it.
add_unicast_pattern (new): reads the operand across the col-major, row-major and arbitrary StoreLayouts, so both the contiguous and strided add_unicast load paths are checked for every store dtype.
Full tract-linalg suite green. Qwen2.5-7B-Instruct-q40ef16 decodes correctly on arm64 CPU (previously NaN); on x86_64 CPU the same kernels carried the latent bug (the add_unicast_dt test fails there without the fix) and the model continues to decode correctly.

Root-caused from #2475; complements #2477 (which gates off the fusion) by making the kernels themselves safe.

🍍

The arm64simd mmm_f32_32x1 and mmm_f32_32x3 kernels declare store(f16) but
their add_unicast path ignored item_size and always read the unicast operand
as f32, so a fused f16 AddUnicast was read as misaligned f32 and could
saturate to inf. Add an item_size==2 branch that converts the operand with
fcvtl before the fadd, mirroring store_f16, plus a store-dtype-gated
add_unicast_dt test that exercises an operand dtype equal to the kernel's
store dtype.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@czoli1976

Copy link
Copy Markdown
Contributor Author

@JulienBalianSonos

kali
kali previously approved these changes Jul 20, 2026
@kali

kali commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

ok ! bug it is.

@kali

kali commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

These are the only two kernels impacted ?

@kali

kali commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

These are the only two kernels impacted ?

nope.

@kali

kali commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

#2477 is superseded, right ?

@JulienBalianSonos

Copy link
Copy Markdown
Collaborator

@czoli1976 Nice finding, congrats! I will let the review go to @kali, as I am not in my comfort zone with assembly, but it 'sounds' like a legit fix.

@czoli1976

czoli1976 commented Jul 20, 2026 via email

Copy link
Copy Markdown
Contributor Author

@github-actions

Copy link
Copy Markdown

⚠️ Bench vs main — no speed regressions · 2 secondary regression(s)

Reference: 2026-07-20 morning nightly run (0d old) · full report → run

Speed — evaltime · prefill · decode

no inference-speed regressions

⚠️ 2 secondary regression(s)
Δ metric device main → PR
⚠️ +15.5% en_tdnn_pyt_15M
load · pulse_120ms
apple-m1-max 84 ms → 97 ms
⚠️ +13.3% en_tdnn_pyt_15M
load+optimize · pulse_120ms
apple-m1-max 98 ms → 111 ms

@kali

kali commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

@czoli1976 are you in a position to address the fma ones, or should I do it ?

@czoli1976

Copy link
Copy Markdown
Contributor Author

@czoli1976 are you in a position to address the fma ones, or should I do it ?

let me look

The 32x1 and 32x3 fma kernels declare f16 store but their add_unicast path
always loaded the unicast operand as f32, ignoring item_size, so a fused f16
operand was read at the wrong width and could saturate to infinity. Add an
item_size == 2 branch that converts f16 operands with vcvtph2ps before the
add, mirroring the f16 store path. Covered by the existing store-dtype-gated
add_unicast_dt test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaAHfyARFifxWDMQTifQbn
add_unicast_dt exercises a single operand layout per kernel. Add an
add_unicast_pattern test that reads the unicast operand in the kernel's
declared output dtype across the col-major, row-major and arbitrary
StoreLayouts, so both the contiguous and strided add_unicast load paths
are checked for every store dtype (guarding the fma f16 fix and the other
kernels' add_unicast).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GaAHfyARFifxWDMQTifQbn
kali
kali previously approved these changes Jul 20, 2026
@github-actions

Copy link
Copy Markdown

⚠️ Bench vs main — no speed regressions · 2 secondary regression(s)

Reference: 2026-07-20 morning nightly run (0d old) · full report → run

Speed — evaltime · prefill · decode

no inference-speed regressions

⚠️ 2 secondary regression(s)
Δ metric device main → PR
⚠️ +15.5% en_tdnn_pyt_15M
load · pulse_120ms
apple-m1-max 84 ms → 97 ms
⚠️ +13.3% en_tdnn_pyt_15M
load+optimize · pulse_120ms
apple-m1-max 98 ms → 111 ms

@kali
kali merged commit e1df296 into sonos:main Jul 20, 2026
54 checks passed
@czoli1976 czoli1976 mentioned this pull request Jul 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants