Run 2D MaxPool on the Metal GPU - #2557
Closed
czoli1976 wants to merge 4 commits into
Closed
Conversation
The nearest-neighbour Resize declutter lowered any integer scale to Reshape -> Tile -> Reshape, but that replication pattern only matches some coordinate transform and tie-break pairs, so half_pixel or ceil rounding silently produced shifted output. Both declutters now probe the rounding of each upsampled axis and lower only when it really is pixel replication.
Resize silently ignored antialias and keep_aspect_ratio_policy, carried axes as a dead field, and rejected half_pixel_symmetric and tf_crop_and_resize, so opset-18 and -19 models were either wrong or refused outright. The op now implements all of them, and resamples through a per-axis plan of precomputed taps and weights applied over contiguous runs rather than recomputing a dynamic-rank index for every output element.
Resize had no Metal kernel, so every node round-tripped the tensor through the host in the middle of a GPU graph. A resample-one-axis kernel now consumes the same per-axis tap-and-weight plan the CPU op builds, which makes it independent of the interpolator; the plan is baked at translation time, so the node keeps only its data input and the scales/sizes TDim constant is dropped.
MaxPool had no Metal kernel, so it round-tripped its tensor through the host in the middle of a GPU graph. A kernel now pools two spatial axes with every axis stride passed explicitly, which covers NCHW and NHWC alike, and skips positions in the padding so an all-padding window keeps the CPU op's lowest-value result. The geometry is resolved at translation time; index outputs, other ranks and symbolic shapes stay on CPU.
czoli1976
marked this pull request as draft
August 2, 2026 18:35
Contributor
Author
|
Marking this draft: it overlaps #2553, which I opened earlier and had not checked before writing this. #2553 already adds Metal max and sum pooling, with a better-coalesced NHWC dispatch. The two are not identical, though — #2553 deliberately leaves NCHW on the CPU path, and this one covers NCHW, which is what an ONNX/PyTorch export like u2netp uses. I will rework the useful part of this as NCHW support on top of #2553 rather than ask anyone to review two pooling kernels. |
Contributor
Author
|
Superseded by #2553, which now carries channels-first max pooling as a second commit. Closing this rather than asking anyone to review two pooling kernels. |
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
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.
MaxPool had no Metal kernel, so it round-tripped its tensor through the host in the middle of a GPU graph. This adds a kernel that pools two spatial axes with every axis stride passed explicitly, so NCHW and NHWC share one path.
Stacked on #2554, #2555 and #2556 — this branch carries their commits and this PR's own change is the last one. It only depends on #2556 for the benchmark story, not for the code, so it can also be taken on its own if that's easier.
Effect
The point of this one is not the kernel's own throughput, it's what it removes. u2netp (rembg), 1×3×320×320,
--metal -O, min of 3 interleaved rounds:DeviceSync*)The graph is now essentially GPU-resident, and that is what makes Metal win on this model: 359.8 ms vs 419.1 ms on CPU. It lost before either kernel (560.7) and still lost with Resize alone (460.7) — the residual host transfers dominated. Resize was necessary, MaxPool was what flipped it. Together, 1.56× on the Metal path.
Outputs match the CPU runtime to 5.96e-7 max abs across all seven model outputs.
Notes
Padding positions are skipped rather than compared, so a window lying entirely in the padding keeps the CPU op's
min_value()result.The translator registers for both
MaxPoolandOptMaxPool. The Metal transform runs before the optimize pass, so the node is stillMaxPoolat translation time even though a-Odump showsOptMaxPool— registering only for the optimized op silently never fires.OptMaxPoolis added tocore::ops::cnn's public re-exports for this.Two limits worth knowing:
with_index_outputsand symbolic shapes aren't translated and stay on CPU.Testing
test-metal22762 passed / 0 failed (558 of them pooling node tests; I checked the nodes actually land onMetalMaxPoolrather than silently falling back to CPU).onnx-tests.sh 1_19_1andtest-unit-coregreen.cargo fmt --allandcargo clippyclean.🍍