Skip to content

metal: max and average pooling kernels - #2553

Merged
kali merged 2 commits into
sonos:mainfrom
czoli1976:feat/metal-pooling
Aug 9, 2026
Merged

metal: max and average pooling kernels#2553
kali merged 2 commits into
sonos:mainfrom
czoli1976:feat/metal-pooling

Conversation

@czoli1976

Copy link
Copy Markdown
Contributor

Stacked on #2552 — its commit is the first of the two here, because the tests
compare against the CPU op and one of them exercises the path that fix repairs.

Pooling has no Metal kernel, so a pooled model leaves the GPU at every pool.
On Inception v3 that is 14 DeviceSyncToHost nodes; profiling it after the
convolution work lands shows the pools themselves at 28% of runtime on the CPU
side and the syncs dominating the rest.

This adds 2D max and sum pooling over channels-last tensors — one thread per
(n, oh, ow, c), so consecutive threads walk the contiguous channel axis and
every window read is coalesced — and routes MaxPool/SumPool and their
optimized forms to them. NCHW, rank ≠ 4, non-float dtypes and max-pool with an
index output stay on the existing path.

After this, Inception v3 has 14 pools on the GPU and its sync count goes
14 → 1.

Numbers

Inception v3 (TF, NHWC, 299×299) on --metal, M1 Pro:

ms/inference
main 478.4
main + this 460.3
#2549+#2550+#2551 (convolutions) 40.6
those + this 22.5

Worth being explicit about that first pair: on main this is close to neutral,
because the direct convolution kernel dominates everything and 16 ms of CPU
pooling is noise beside it. The gain only appears once convolution is off the
critical path, where it is 1.80×. If the convolution PRs are not wanted,
this one is not worth much on its own.

Output matches the CPU path (same predicted class, max difference 3e-7).

Validation

cargo test -p tract-metal --release: 83 passed, one failure —
test_mfa_attention_causal_const_is_noop, pre-existing on main and unrelated
(#2546). New tests cover max pooling with valid and same padding, average
pooling with and without count_include_pad, un-normalized sum pooling, f16,
and an end-to-end case asserting both pools land on the GPU and match CPU.
fmt and clippy clean.

One wart worth flagging for review: the translators live in ops::pool, which
nothing else calls into, and the linker drops the module — and with it the
inventory registrations — unless something references it. There is a
link_translators() call in the transform to hold it. If there is an idiom you
prefer for that, say so.

M1 Pro only; my M4 was asleep.

🍍

@czoli1976

Copy link
Copy Markdown
Contributor Author

Added a second commit: channels-first max pooling.

The kernels here only covered channels-last, so a channels-first model — what an ONNX or PyTorch export gives — kept every max pool on the CPU. The new kernel takes the width axis as the fastest-moving one so its reads stay coalesced the way the channels-last one's do. Sum pooling stays channels-last.

On u2netp (rembg, ONNX/PyTorch so NCHW, 1x3x320x320, --metal -O, min of 3 interleaved rounds) its 33 max pools move to the GPU, and it goes 743.4 -> 718.0 ms. That is only 1.04x, for the same reason this PR already notes about itself: the model's 38 Resize nodes are still on the CPU here and their syncs dominate. Stacked with #2556 (Metal Resize) the same change is worth 460.7 -> 359.8 ms, which is where u2netp's Metal path overtakes its CPU path (359.8 vs 419.1 ms).

Output matches the CPU path to 1.8e-7 max abs. Two new unit tests cover channels-first max pooling in f32 and f16, valid and same-padding. test-metal 22348 passed / 0 failed, tract-metal unit tests 86 passed, onnx-tests.sh green, fmt and clippy clean.

This supersedes #2557, which I opened before noticing this PR existed; closing that one.

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

🔴 Bench vs main — 7 speed regression(s) · ⚠️ 17 secondary

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

Speed — evaltime · prefill · decode

Δ metric device main → PR
🔴 +12.9% hey_snips_v4_model17
evaltime · 2sec
apple-m1-max 0.868 ms → 0.98 ms
🔴 +7.2% hey_snips_v1
evaltime · 400ms
apple-m1-max 0.121 ms → 0.13 ms
🔴 +6.8% arm_ml_kws_cnn_m
evaltime · pass
cortex-a9 9.2 ms → 9.82 ms
🔴 +5.3% hey_snips_v4_model17
evaltime · pulse8
apple-m1-max 0.11 ms → 0.116 ms
🔴 +5.1% hey_snips_v4_model17_nnef
evaltime · pulse8
apple-m1-max 0.11 ms → 0.116 ms
+2 more regression(s)
Δ metric device main → PR
🔴 +4.8% openelm_270M_q40ef16_541
decode · cpu
apple-m1-max 12.1 ms/tok
82.85 tok/s → 12.7 ms/tok
79.05 tok/s
🔴 +4.1% openelm_270M_q40ef16_541
prefill · cpu
apple-m1-max 3.23 ms/tok
309.4 tok/s → 3.36 ms/tok
297.3 tok/s
⚠️ 17 secondary regression(s)
Δ metric device main → PR
⚠️ +42.9% arm_ml_kws_cnn_m
load · pass
cortex-a9 84 ms → 120 ms
⚠️ +40.3% arm_ml_kws_cnn_m
load+optimize · pass
cortex-a9 129 ms → 181 ms
⚠️ +22.2% hey_snips_v1
load+optimize · 400ms
cortex-a9 144 ms → 176 ms
⚠️ +21.2% parakeet_tdt_600m_v3_f32f32_decoder_pass
load+optimize · cpu
apple-m1-max 33 ms → 40 ms
⚠️ +21.1% hey_snips_v1
load · 400ms
cortex-a9 76 ms → 92 ms
⚠️ +18.6% en_tdnn_15M_nnef
load+optimize · pulse_240ms
apple-m1-max 43 ms → 51 ms
⚠️ +17.6% en_tdnn_8M_nnef
load+optimize · pulse_240ms
apple-m1-max 34 ms → 40 ms
⚠️ +17.1% parakeet_tdt_600m_v3_f32f32_joint_pass
load+optimize · cpu
i9-11900kb_rtx-4060 41 ms → 48 ms
⚠️ +12.5% hey_snips_v4_model17_nnef
load · pulse8
apple-m1-max 32 ms → 36 ms
⚠️ +9.7% en_tdnn_15M_nnef
load · pulse_240ms
apple-m1-max 31 ms → 34 ms
⚠️ +8.5% arm_ml_kws_cnn_m
load · pass
cortex-a53 47 ms → 51 ms
⚠️ +8.0% hey_snips_v4_model17_nnef
load+optimize · pulse8
apple-m1-max 50 ms → 54 ms
⚠️ +6.7% en_tdnn_15M
load+optimize · 2600ms
apple-m1-max 89 ms → 95 ms
⚠️ +6.3% en_tdnn_15M
RSS @ ready · 2600ms
cortex-a55 110 MB → 118 MB
⚠️ +6.3% voicecom_float
load+optimize · 2sec
cortex-a53 160 ms → 170 ms
⚠️ +5.5% arm_ml_kws_cnn_m
load+optimize · pass
cortex-a53 73 ms → 77 ms
⚠️ +5.2% voicecom_fake_quant
load+optimize · 2sec
cortex-a53 232 ms → 244 ms

czoli1976 and others added 2 commits August 8, 2026 19:02
Pooling had no Metal kernel, so a pooled model bounced back to the host
at every pool. On Inception v3 that is 14 device syncs and, once the
convolutions are on the GPU, most of what is left: the pools alone were a
fifth of the runtime on the CPU side. Add 2D max and sum pooling over
channels-last tensors and route MaxPool/SumPool - and their optimized
forms - to them. Anything else, including NCHW and rank other than 4,
stays where it was.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pooling kernels only covered channels-last tensors, so a channels-first
model — what an ONNX or PyTorch export gives — kept every max pool on the CPU.
A channels-first max pool now takes the width axis as the fastest-moving one,
so its reads stay coalesced the way the channels-last kernel's do. Sum pooling
stays channels-last.
@kali

kali commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

⚠️⚠️⚠️ Just rebased! ⚠️⚠️⚠️

@kali
kali force-pushed the feat/metal-pooling branch from 7464c30 to 6bdc351 Compare August 8, 2026 17:02
@kali

kali commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator

oups, sorry!

@github-actions

github-actions Bot commented Aug 9, 2026

Copy link
Copy Markdown

🔴 Bench vs main — 1 speed regression(s) · ⚠️ 3 secondary

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

Speed — evaltime · prefill · decode

Δ metric device main → PR
🔴 +8.1% arm_ml_kws_cnn_m
evaltime · pass
cortex-a9 9.21 ms → 9.95 ms
⚠️ 3 secondary regression(s)
Δ metric device main → PR
⚠️ +47.6% arm_ml_kws_cnn_m
load · pass
cortex-a9 84 ms → 124 ms
⚠️ +39.2% arm_ml_kws_cnn_m
load+optimize · pass
cortex-a9 130 ms → 181 ms
⚠️ +6.4% en_tdnn_15M
RSS @ ready · 2600ms
cortex-a55 111 MB → 118 MB

@kali
kali merged commit d608f6a into sonos:main Aug 9, 2026
58 checks passed
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.

2 participants