Metal backend: keep non-packed views in their parent's buffer by abdelaziz-mahdy · Pull Request #22984 · pytorch/executorch

abdelaziz-mahdy · 2026-09-22T00:03:01Z

Summary

Stacked on #22957, which adds the view registration this builds on; only the last commit (f669c51dc1) is new. Once #22957 lands the diff reduces to that commit.

aoti_torch__reinterpret_tensor replaced a view that is not densely packed with a packed copy. Kernels generated by inductor index with the strides they were compiled with and write through such views as well as read them, so against a packed copy they lose their writes and index up to parent numel into a buffer of view numel. Inductor's default channels-last layout makes this the common case: a chunk along C is non-packed there, and a cat is filled through views of it.

A non-packed view of a Metal buffer now stays where it is and is registered as a view of its parent, like a packed one. The tensor cannot describe such strides (make_tensor_ptr rejects them), so it carries packed ones and the real ones are recorded with metal_record_strided_view.
ETMetalKernelFunction::setArg takes how the kernel uses the argument (ArgAccess). aoti_torch_mps_set_arg_tensor passes kStridedInPlace: a generated kernel gets the view where it is, which is all it needs.
Hand-written ops need dense input. get_mtl_buffer (mm, addmm, bmm, convolution, topk) and setArg with kRead hand them a packed copy, made by a small gather kernel encoded on the stream when the op runs. Nothing is committed or waited for, and the op's own encoder stays open. An argument a hand-written kernel writes (kWrite) is refused if it is a strided view, and so is a view that cannot be packed.
An MPSGraph op asked to write its result through such a view, and aoti_torch_copy_ into one, now fail instead of writing to the wrong place. aoti_torch_copy_ from one copies through the same gather; the backend uses it to copy a model's outputs out, so a model can return such a view.
Non-packed views of CPU memory are materialized as before. materialize_packed loses its device path, which nothing reaches any more.
Every handle into memory the runtime owns now holds a count on the allocation it lives in, including views of views and handles copied from views, and gives it back when deleted. Before, a view at an offset never gave its count back (the parent was not freed until cleanup), and nested or copied views took none (the parent could be freed under them). This predates the PR, but the PR sends more views down that path.

This resolves the "Not fixed here" item of #22957.

Test plan

pointwise_c2f added (the YOLO C2f block reduced to 1x1 convs), linear_chunk_last_dim_output added (the model's output is such a view), and linear_chunk_cat_last_dim enabled; it was skipped for this reason.

backends/apple/metal/tests/run_metal_test.sh --build
python -m unittest backends.apple.metal.tests.test_modules.TestMetalBackendModules

Without the runtime change: pointwise_c2f fails with max_atol=0.586 (float32) and 0.755 (bfloat16), linear_chunk_cat_last_dim with max_atol=0.518.
With it: Ran 142 tests, all passing except the 10 int4 tests that fail on macOS 27.0 for an unrelated reason (Metal backend: 4-bit linear shader fails to compile with the macOS 27 Metal compiler #22981, fixed by Metal backend: restore the thread qualifiers in the 4-bit matmul shader #22982). test_metal_memory 11 of 11 and new test_metal_strided_view 6 of 6; each test added for a review finding fails without its change.
End to end, on a build that also has Metal backend: honor channels-last strides in conv2d #22952 and the int4 fix (Ran 166 tests ... OK), all-ones input against eager through executor_runner: yolov8n exported with the default layout went from crashing 10 runs out of 10 to max abs diff 0.0018 over 5 runs; with layout_optimization=False it stays at 0.0019. yolo11n with the default layout still stops in aoti_torch_mps_bmm_out, which rejects a permuted input ([2,64,400], strides [1,800,2]); that is a separate limitation.
lintrunner clean.

…set view aoti_torch__reinterpret_tensor read views that start partway into a buffer before the GPU had written them. A non-packed view is copied on the CPU, and the wait for pending GPU work was guarded by metal_is_device_pointer(src). src is the offset pointer, and only a buffer's base address is registered, so the wait was skipped whenever the storage offset was non-zero. A packed view gets its own no-copy MTLBuffer over the parent's memory; Metal tracks hazards per buffer object, so nothing ordered work reading it after the pending work that writes the parent. Decide on the wait from the base pointer, and wait before aliasing a packed view. Adds two linear-then-chunk modules that fed the second chunk to another linear and came back as zeros.

…aliasing it aoti_torch__reinterpret_tensor gave every packed view with a storage offset its own no-copy MTLBuffer over the parent's memory. Metal treats the two buffers as unrelated: a write through one followed by a read of the other in the same serial compute encoder sees stale data every time. Inductor does exactly that when it fills the result of a cat by writing through views of it, and the opposite when an op reads a chunk of a buffer another op just wrote, so models built on split/cat (YOLO's C2f blocks) ran but were wrong. Register such a view against the buffer it lives in and bind that buffer at the view's offset, in compute encoders and in blits. This replaces the wait-before-aliasing from the previous commit, which only covered reads. MPSGraphTensorData cannot address into a buffer, so MPSGraph ops still get an alias for an offset view, now with the stream synchronized on both sides of the graph. Adds cat variants of the chunk tests. The last-dim one is skipped: its slices are non-packed views, which reinterpret_tensor materializes into a copy, so writes through them are still lost.

Inductor flattens nested views into a single reinterpret of the base buffer with the combined offset, so this exercises a larger packed offset rather than a view of a view, but it pins that behavior down.

…retain

A view registration is counted, but only aoti_torch__reinterpret_tensor with a non-zero offset took a count. aoti_torch_new_tensor_handle on a view, and a reinterpret of a view at offset 0, both make another handle at the same address without one, while aoti_torch_delete_tensor_object gives one back for every handle. Deleting either handle therefore unregistered the view for the other: a kernel writing through the surviving handle fell back to a temporary and the write never reached the buffer, and MPSGraph ops failed to find the tensor's Metal buffer. metal_retain_view takes a count for an address that is a registered view and does nothing otherwise; both paths call it. Tests cover both ways of making the second handle, in both deletion orders. They need the test_metal_memory target, hence the merge of main.

get_mtl_buffer asked the stream to wait after the next graph before it knew the alias MTLBuffer could be created. On failure it throws, and the flag would have made the next, unrelated graph wait. Also reword a comment in materialize_packed that claimed only base addresses are known as device pointers; constant sub-buffers are mapped at interior addresses too.

aoti_torch__reinterpret_tensor replaced a view that is not densely packed with a packed copy. Kernels generated by inductor do not look at the tensor they are handed: they index with the strides they were compiled with, and they write through such views as well as read them. Inductor's default channels-last layout makes this the common case, since a chunk along C is non-packed there and a cat is filled through four views of it. Against a packed copy those kernels lose their writes, so the cat buffer stays empty, and index up to `parent numel` into a buffer of `view numel`, which is an out-of-bounds GPU access: a C2f block returns wrong values, and yolov8n crashes inside MPSGraph once the heap has been written over. A non-packed view of a Metal buffer now stays where it is and is registered as a view of its parent, like a packed one. The tensor cannot describe its strides, so it carries packed ones and the real ones are recorded (metal_record_strided_view). aoti_torch_mps_set_arg_tensor binds the view in place. Hand-written ops need dense input, so get_mtl_buffer and ETMetalKernelFunction::setArg hand them a packed copy made when the op runs; an MPSGraph op asked to write its result through such a view, and aoti_torch_copy_ on one, fail instead of writing to the wrong place. Non-packed views of CPU memory are materialized as before. Adds pointwise_c2f, the YOLO C2f block reduced to 1x1 convs, and enables linear_chunk_cat_last_dim, which was skipped for this reason.

pytorch-bot · 2026-09-22T00:03:05Z

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22984

📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⚠️ 23 Awaiting Approval

As of commit ca9447b with merge base 9b91b43 ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

github-actions · 2026-09-22T00:03:50Z

This PR needs a `release notes:` label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

Copilot

Copilot review overview

🟡 Changes recommended

A critical encoder-lifecycle issue and a moderate view-reference lifetime issue remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

Strided argument packing invalidates the active Metal encoder · New
Offset view deletion leaks the owning buffer reference · New

What changed in this PR

Updates the Metal backend to preserve non-packed views in parent buffers while supplying dense copies to operations that require them.

Changes:

Adds Metal view registration, binding, and lifetime tracking.
Adds packed-copy handling for dense-only operations.
Adds regression and view-lifetime tests.

File	Reviewed change
`backends/apple/metal/tests/test_modules.py`	Adds view-layout regressions. Nit (1 vote): update the misleading docstring about materialization.
`backends/apple/metal/runtime/test/test_memory.cpp`	Tests view-handle lifetime tracking.
`backends/apple/metal/runtime/shims/shim_mps.mm`	Binds generated kernels directly to view storage.
`backends/apple/metal/runtime/shims/memory.cpp`	Preserves views and validates copies. Moderate (3 votes): owning-buffer references for offset views are not released.
`backends/apple/metal/runtime/shims/et_metal.mm`	Implements view resolution and packed copies. Critical (3 votes): synchronization can end the active encoder before packed-buffer binding and dispatch.
`backends/apple/metal/runtime/shims/et_metal.h`	Exposes view and synchronization APIs.
`backends/apple/metal/runtime/ops/common.mm`	Provides dense buffers for MPSGraph operations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Two problems in the previous commit, both found in review. The packed copy that hand-written ops get of a strided view was made on the CPU after a COMMIT_AND_WAIT. ETMetalKernelFunction::setArg runs after the op has started encoding, and synchronize() ends the stream's encoder, so the op went on binding and dispatching through an encoder that had been ended. The copy is now a gather kernel encoded on the stream's current encoder: nothing is committed or waited for, the copy is ordered after the work that fills the view, and the op's own pipeline state is set again afterwards. It binds at the top of the argument table so the arguments the op has already set stay put. A view at another address than its parent takes a count on the parent's memory, but deleting the view only unregistered it, so the parent's count never came back down and its buffer was not freed until cleanup. The shims now remember which parent each such view counts towards and give the count back when the view is deleted. This predates the previous commit; that commit makes more views take this path. Adds test_metal_strided_view (the view stays in its parent, the GPU gather returns its elements, a copied handle is a strided view too) and two tests to test_metal_memory that fail without the refcount change.

Copilot

Copilot review overview

🟡 Changes recommended

Critical issues remain in packed-view fallback/output handling and nested or copied-view lifetime ownership.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 · 1

Open (5)

Handle failed view packing instead of binding invalid strides · New
Avoid packing writable outputs and reject failed gathers · New
Track ownership across copied offset-view handles · New
Retain root ownership for nested tensor views · New
Update docstring to reflect preserved Metal view behavior · New

Resolved since last review (2)

Strided argument packing invalidates the active Metal encoder
Offset view deletion leaks the owning buffer reference

…memory alive Review of the previous commit found four problems, and a fifth came up checking the rest of the change. - setArg packed every strided argument, including ones the kernel writes, and when packing failed it bound the view with the packed strides it carries. setArg now takes how the kernel uses the argument: a hand-written kernel's input gets a packed copy, an argument it writes is refused if it is a strided view (writes to a copy would be lost), and a kernel generated by inductor gets the view in place. A view that cannot be packed is an error. The outputs of the hand-written kernels are allocated by the ops and are never views; gated_delta_rule's state, updated in place, is the one caller tensor they write. - A view of a view took no count on the allocation it lives in, and a handle copied from a view took none either, so deleting the base and the first handle could free memory a remaining handle still pointed into. Every handle into memory the runtime owns now holds a count on that allocation, found through the handle it was made from. - aoti_torch_copy_ refused strided views, which broke models whose output is one: the backend copies outputs out with it. A strided source is now copied through the same GPU gather. Adds linear_chunk_last_dim_output, tests for nested views and copied view handles in test_metal_memory, and tests for the argument rules in test_metal_strided_view. Test docstrings no longer describe views as materialized.

abdelaziz-mahdy · 2026-09-22T01:15:22Z

Pushed b48b581 for the five review comments above. Going over the rest of the change I also found a regression the reviews did not cover: 4648c9a made aoti_torch_copy_ refuse non-packed views, and the backend copies a model's outputs out with it, so a model returning such a view (e.g. linear(x).chunk(2, dim=-1)[1]) failed with aoti_torch_copy_ does not support views that are not densely packed. A strided source is now copied through the same GPU gather, and linear_chunk_last_dim_output covers it; it fails on 4648c9a and passes now.

Copilot

Copilot review overview

🔵 Needs a closer look

The broad Metal runtime and ownership changes warrant final human review.

Review effort: Lite
Findings: None

Resolved since last review (5)

Retain root ownership for nested tensor views
Track ownership across copied offset-view handles
Avoid packing writable outputs and reject failed gathers
Handle failed view packing instead of binding invalid strides
Update docstring to reflect preserved Metal view behavior

…iews working Review of 89dbe34 found two problems, and a third came up checking the memory accounting around them. - The wait that an aliasing buffer needs was a flag on the stream, armed by get_mtl_buffer and consumed by whichever graph ran next. Another thread's graph could consume it, and an op failing between get_mtl_buffer and its graph left it armed for an unrelated one. get_mtl_buffer now only reports that it made an alias, the op passes that to its own executeMPSGraph call, and the stream waits before and after encoding that graph inside the same block on its serial queue, so nothing else can come in between. The stream no longer holds any such state. - A packed offset view of CPU memory lost the no-copy buffer it used to get, so MPSGraph ops could not find it and kernels wrote to a temporary. It gets one again. Unlike before, the buffer is counted with the view's handles and released with the last of them, so that it does not outlive the CPU memory under it; a longer view at the same address gets a buffer that covers it. - A view at an offset took a count on its parent allocation that deleting it never gave back, and a view of a view, or a handle copied from a view, took none. The parent was then either never freed, or freed while such a handle still pointed into it. Every handle into memory the runtime owns now holds a count on the allocation it lives in, found through the handle it was made from, and gives it back when deleted. test_metal_memory: 9 new tests. All but AliasedGraphSettlesItsOwnWork fail on 89dbe34; that one covers what the change must keep working.

…-views Brings in the per-graph alias wait, the counted buffers of CPU-backed views, and the ownership accounting this branch already had. Two things needed changing for them to work together: - A view of CPU memory that has a Metal buffer of its own counts as a device pointer, but it cannot hold a non-packed view in place: its buffer covers only itself. Such a view is materialized, like any view of CPU memory. - materialize_packed waits for the GPU again before reading. Kernels can now write CPU memory through the buffers of views of it, and a copy made afterwards has to see those writes. test_metal_strided_view gains a test for each; both fail without the change.

abdelaziz-mahdy · 2026-09-22T22:18:10Z

Merged the updated #22957 (67d8d5d) into this branch as 878aa22. The ownership accounting that was here now comes from #22957. Two things needed changing for the two to fit together:

Metal backend: bind offset views to their parent's buffer instead of aliasing it #22957 gives a packed offset view of CPU memory its own no-copy buffer again. Such a view counts as a device pointer, but it cannot hold a non-packed view in place, since its buffer covers only itself. A non-packed view of it is now materialized, like any view of CPU memory (NonPackedViewOfCpuBackedViewIsMaterialized).
materialize_packed waits for the GPU again before reading: kernels can write CPU memory through those buffers, and a copy made afterwards has to see the writes (MaterializingCpuMemoryWaitsForGpuWrites).

Both tests fail without the change. test_metal_memory 16/16, test_metal_strided_view 8/8, module suite 142 run with only the 10 int4 failures (#22982, merged, not in this branch), lintrunner clean.

Copilot

Copilot review overview

🔵 Needs a closer look

Three header-include nits remain, and the broad runtime changes warrant human approval.

Review effort: Lite
Findings: 1

Open (1)

Add direct <algorithm> include for std::fill_n · New

metal_copy_memory copied with memcpy and only then waited for the GPU, and only when the source was on the device. A copy to the host could read what a pending command buffer had not written yet, and a copy to the device could overwrite memory a pending command buffer was still to read. It now waits before the copy whenever either side is on the device; with nothing pending the wait does nothing. The backend itself waits before copying a model's outputs, so exported models were not affected; direct aoti_torch_copy_ calls were. Two tests, one per direction, fail without the change.

test_strided_view.mm used std::fill_n and std::shared_ptr, et_metal.mm UINT32_MAX, std::runtime_error and std::to_string in the new gather and setArg code, and common.mm std::strcmp, all through transitive includes.

Copilot

Copilot review overview

🔵 Needs a closer look

The broad Metal runtime and operator changes warrant final human review.

Review effort: Lite
Findings: None

Resolved since last review (1)

Add direct <algorithm> include for std::fill_n

abdelaziz-mahdy added 7 commits September 19, 2026 23:03

Cover a chunk of a chunk in the Metal offset-view tests

0a4a1f5

Inductor flattens nested views into a single reinterpret of the base buffer with the combined offset, so this exercises a larger packed offset rather than a view of a view, but it pins that behavior down.

Merge remote-tracking branch 'upstream/main' into local/offset-views-…

4ad8cea

…retain

Copilot AI lite review requested due to automatic review settings September 22, 2026 00:03

abdelaziz-mahdy requested a review from shoumikhin as a code owner September 22, 2026 00:03

abdelaziz-mahdy mentioned this pull request Sep 22, 2026

Metal backend: bind offset views to their parent's buffer instead of aliasing it #22957

Open

meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 22, 2026

Copilot started reviewing on behalf of abdelaziz-mahdy September 22, 2026 00:03 View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Comment thread backends/apple/metal/runtime/shims/et_metal.mm Outdated

Comment thread backends/apple/metal/runtime/shims/memory.cpp Outdated

Copilot AI review requested due to automatic review settings September 22, 2026 00:28

abdelaziz-mahdy requested review from kirklandsign and larryliu0820 as code owners September 22, 2026 00:28

Copilot started reviewing on behalf of abdelaziz-mahdy September 22, 2026 00:29 View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings September 22, 2026 01:14

Copilot started reviewing on behalf of abdelaziz-mahdy September 22, 2026 01:15 View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

nil-is-all added the module: metal Issues related to the AOTI Metal backend label Sep 22, 2026

executorch-triage Bot added the community: contribution PRs coming from community (excluding hardware partners) label Sep 22, 2026

abdelaziz-mahdy added 2 commits September 22, 2026 19:09

Copilot AI review requested due to automatic review settings September 22, 2026 22:18

Copilot started reviewing on behalf of abdelaziz-mahdy September 22, 2026 22:18 View session

Copilot AI reviewed Sep 22, 2026

View reviewed changes

Comment thread backends/apple/metal/runtime/test/test_strided_view.mm

abdelaziz-mahdy added 3 commits September 22, 2026 20:44

Merge branch 'fix/metal-offset-views' into fix/metal-nonpacked-views

bc82404

Include what the Metal view code uses directly

ca9447b

test_strided_view.mm used std::fill_n and std::shared_ptr, et_metal.mm UINT32_MAX, std::runtime_error and std::to_string in the new gather and setArg code, and common.mm std::strcmp, all through transitive includes.

Copilot AI review requested due to automatic review settings September 22, 2026 23:51

Copilot started reviewing on behalf of abdelaziz-mahdy September 22, 2026 23:51 View session

Copilot AI reviewed Sep 23, 2026

View reviewed changes

Navigation Menu

Metal backend: keep non-packed views in their parent's buffer - #22984

Metal backend: keep non-packed views in their parent's buffer#22984
abdelaziz-mahdy wants to merge 14 commits into
pytorch:mainfrom
abdelaziz-mahdy:fix/metal-nonpacked-views

abdelaziz-mahdy commented Sep 22, 2026 •

edited

Loading

Uh oh!

pytorch-bot Bot commented Sep 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abdelaziz-mahdy commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

abdelaziz-mahdy commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

abdelaziz-mahdy commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

pytorch-bot Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22984

⚠️ 23 Awaiting Approval

Uh oh!

github-actions Bot commented Sep 22, 2026

This PR needs a release notes: label

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

abdelaziz-mahdy commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

abdelaziz-mahdy commented Sep 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abdelaziz-mahdy commented Sep 22, 2026 •

edited

Loading

pytorch-bot Bot commented Sep 22, 2026 •

edited

Loading

This PR needs a `release notes:` label