You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Ops consuming or filling a view that starts partway into a Metal buffer got stale data, silently. Anything built on chunk / split / cat is affected; the YOLO family (C2f blocks) ran but returned box coordinates off by hundreds.
Packed views with a storage offset.aoti_torch__reinterpret_tensor gave each one its own no-copy MTLBuffer over the parent's memory (metal_buffer_nocopy). Metal treats the two buffers as unrelated, and that is not just an ordering hint: a write through the alias followed by a read of the parent in the same serial compute encoder sees stale data 200 times out of 200 in a standalone Metal program, while binding the parent at an offset is correct 200 out of 200 (repro in the issue). Inductor does both directions: an op reads a chunk of a buffer another op just wrote, and the result of a cat is filled by writing through views of it (buf6/7/8 = reinterpret_tensor_wrapper(buf9, ..., offset) // alias).
Such a view is now registered against the buffer it lives in (metal_register_view) and resolved to that buffer plus a byte offset (metal_resolve_buffer), which is what ETMetalKernelFunction::setArg, metal_copy_memory and metal_is_device_pointer use. Registrations are counted, since several tensors can be views of one address, and released when the view tensor is deleted.
MPSGraphTensorData cannot address into a buffer, so get_mtl_buffer (mm, addmm, bmm, convolution, topk) still hands MPSGraph an alias for an offset view, but now synchronizes the stream before creating it and has the stream wait again after the graph runs, so the memory is settled on both sides. On macOS 15+ MPSNDArray initWithBuffer:offset:descriptor: would remove the need for that; left for a follow-up.
Non-packed views are copied on the CPU by materialize_packed, which waits for pending GPU work only if metal_is_device_pointer(src). src was the offset pointer, which was never registered, so the wait was skipped for every non-zero offset. The caller now says where the owning buffer lives, decided from the base pointer.
Not fixed here: writes through a non-packed view are still lost, because the view is materialized into a copy. linear_chunk_cat_last_dim covers it and is registered with a skip reason. It needs the materialization to move out of reinterpret_tensor (generated kernels index by stride and only need the right base address; it is the MPSGraph ops that need dense input), which is a larger change I would rather discuss first.
Test plan
Five modules added to MODULE_REGISTRY in backends/apple/metal/tests/test_modules.py, each in float32 and bfloat16: linear_chunk_last_dim (read of a non-packed view), linear_chunk_first_dim (read of a packed view), linear_chunk_cat_first_dim (writes through packed views, then a read of the parent), linear_nested_chunk (a chunk of a chunk, which inductor flattens into a single offset view of the base buffer), linear_chunk_cat_last_dim (skipped, see above).
On main: the linear_chunk_* output tests fail with max_rtol=1.0 (the consumer read zeros).
With only the base-pointer check: the last_dim read passes, the rest still fail, which is what separated the causes.
With this PR: Ran 130 tests ... OK (skipped=4) for the full suite before linear_nested_chunk was added; its 4 tests were then run on their own and pass.
End to end, on a build that also has Metal backend: honor channels-last strides in conv2d #22952, with models exported under torch._inductor.config.layout_optimization = False: yolo11n, yolov8n and yolov5n went from a max abs difference of 340 to 515 against their XNNPACK exports (same input, values up to ~640) to 0.002. Against eager on an all-ones input through executor_runner: yolov8n 0.0019, yolo11n 0.0020. With inductor's default layout optimization these models still do not run correctly (unsupported bmm input layout in yolo11n, and the non-packed write-through limitation above).
…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.
Note: Links to docs will display an error until the docs builds have been completed.
This comment was automatically generated by Dr. CI and updates every 15 minutes.
meta-claBot
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 20, 2026
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"
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟢 Approval recommended
The changes directly address the documented hazard/synchronization gaps and include focused regression coverage for both identified failure modes.
Review effort: Lite Findings: None
What changed in this PR
This PR fixes a correctness bug in the Apple Metal backend where ops could read from a view with a non-zero storage offset before the parent buffer’s GPU writes had completed, producing silently wrong results (often zeros). It tightens synchronization in aoti_torch__reinterpret_tensor for both the “materialize non-packed view” path and the “packed offset view via alias MTLBuffer” path, and adds targeted regression modules to the Metal test suite.
Changes:
Ensure materialize_packed waits for pending GPU writes based on the owning buffer’s base pointer being a Metal-mapped pointer (not the potentially-offset src pointer).
For packed offset views, synchronize the Metal stream before creating a no-copy alias MTLBuffer over the parent’s memory so Metal hazard tracking cannot reorder alias reads ahead of parent writes.
Add two Metal backend test modules that specifically exercise offset views produced by chunk along last-dim (non-packed) and first-dim (packed) cases.
…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.
abdelaziz-mahdy
changed the title
Metal backend: wait for the parent's GPU writes before reading an offset view
Metal backend: bind offset views to their parent's buffer instead of aliasing it
Sep 20, 2026
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
The new view registration fixes correctness, but the current view deletion/refcounting path appears incomplete (base allocation refcounts aren’t decremented for views), and executeMPSGraph ignores its syncType argument, which can leave graph work uncommitted contrary to caller intent.
Get a fresh assessment by requesting another Copilot review.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this fix for #22956! Directing offset views back to their parent MTLBuffer addresses the silent zero reads. Before this lands, please update the PR with the following changes:
1.Unit Test Coverage: Please add a regression test in backends/apple/metal/tests/test_modules.py covering sliced/chunked linear outputs (chunk, split, and unbind) to ensure this setup doesn't break in future releases.
Nested Views Check: Ensure the offset calculation correctly handles nested views (e.g., calling .chunk() or .narrow() on a tensor that is already an offset view of a base buffer).
Once those tests are added, this will be ready to approve!
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.
The tests are already part of this PR, in backends/apple/metal/tests/test_modules.py: linear_chunk_last_dim (read of a non-packed view), linear_chunk_first_dim (read of a packed view), linear_chunk_cat_first_dim (writes through packed views, then a read of the parent) and linear_chunk_cat_last_dim (registered with a skip reason, see the "Not fixed here" section). Each runs in float32 and bfloat16, and they fail on main. split and unbind reach the runtime exactly like chunk, as reinterpret_tensor_wrapper(buf, sizes, strides, storage_offset), so they exercise the same code.
Nested views: added linear_nested_chunk in 0a4a1f5 (a chunk of a chunk of a linear's output, fed to another linear); it passes. Worth noting that inductor flattens these before they get here, the generated wrapper contains a single reinterpret_tensor_wrapper(buf0, 2, ..., 144LL) against the base buffer with the combined offset, so the runtime does not see a view of a view from generated code. metal_register_view still resolves a view whose parent is itself a registered view to the root buffer, for callers that do nest.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this fix for #22956! Directing offset views back to their parent MTLBuffer addresses the silent zero reads. Before this lands, please update the PR with the following changes:
Unit Test Coverage: Please add a regression test in backends/apple/metal/tests/test_modules.py covering sliced/chunked linear outputs (chunk, split, and unbind) to ensure this setup doesn't break in future releases.
Nested Views Check: Ensure the offset calculation correctly handles nested views (e.g., calling .chunk() or .narrow() on a tensor that is already an offset view of a base buffer)
Once those tests are added, this will be ready to approve!
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
syncAfterNextGraph_ is a non-atomic shared flag set/read without synchronization, which can race in multi-threaded use of the singleton ETMetalStream and cause the wrong MPSGraph call to be waited (or miss the intended wait).
Get a fresh assessment by requesting another Copilot review.
syncAfterNextGraph_ is a plain bool that’s written in syncAfterNextGraph() and read/reset in executeMPSGraph() without any synchronization. Since ETMetalStream is a shared singleton (and already uses serialQueue_/atomics for thread-safety elsewhere), this can race across threads and cause the wrong MPSGraph call to be forced to COMMIT_AND_WAIT (or miss the intended one). Consider making it std::atomic<bool> (e.g., set via store(true) and consume via exchange(false)), or guarding both the setter and the consume/reset with serialQueue_.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the detailed breakdown and for adding linear_nested_chunk in 0a4a1f5! That clarifies how Inductor flattens the wrappers down to the root buffer. Everything looks solid—LGTM!
Found a blocking registration-lifetime regression at 0a4a1f56818174581c25157e94a9b6aea4548cd0: deleting one tensor handle can remove the Metal registration while another handle still refers to the same offset view.
The new metal_unregister_view(data_ptr) in aoti_torch_delete_tensor_object runs for every non-owning tensor. However, aoti_torch__reinterpret_tensor registers only when adjusted_data != data_ptr, and aoti_torch_new_tensor_handle does not retain registration. Both same-address alias creation paths therefore leave the registration count too low.
Minimal repro sequence using the current shims:
int64_t base_size = 8, view_size = 4, stride = 1;
AOTITensorHandle base = nullptr, view = nullptr, alias = nullptr;
// float32, MPSaoti_torch_empty_strided(1, &base_size, &stride, 6, 13, 0, &base);
std::fill_n(static_cast<float*>(base->mutable_data_ptr()), 8, 0.0f);
aoti_torch__reinterpret_tensor(base, 1, &view_size, &stride, 4, &view);
aoti_torch_new_tensor_handle(view, &alias);
// Also reproduces if the previous call is replaced with:// aoti_torch__reinterpret_tensor(view, 1, &view_size, &stride, 0, &alias);aoti_torch_delete_tensor_object(view);
EXPECT_TRUE(metal_is_device_pointer(alias->mutable_data_ptr())); // fails
This has observable execution consequences:
A Metal kernel writing 7 through the surviving alias leaves base[4] at 0. ETMetalKernelFunction::setArg takes its CPU fallback and binds a temporary, so the write never reaches the original storage. Both alias-creation paths fail with either deletion order: four failing cases on the PR, all passing on baseline.
Passing a surviving duplicated 2×2 view to aoti_torch_mps_mm_out fails with “self tensor not found in Metal buffer mapping.” This fifth case also passes on baseline.
Please retain registration for both same-address alias creation paths and balance release against every handle's deletion. A local diagnostic change adding those retains fixes all five failures, while ten other native checks covering nested offsets, copies, pending GPU writes, and MPSGraph alias handling continue to pass. These cases should accompany the fix as regression tests.
The broader validation does confirm useful fixes: all eight enabled added module/dtype cases pass, and the last-dimension chunk failure reproduces on baseline and is fixed. The two explicitly skipped non-packed-write cases still fail on both runtimes, consistent with the documented limitation.
I could not corroborate the full-model YOLO numbers with independently exported pretrained models. With #22952's convolution implementation held constant across all runtime variants, YOLO11 hits an unchanged unsupported BMM layout ([2,64,400], strides [1,800,2]), while YOLOv8 completes with maximum box-coordinate error 357.46594 against eager on baseline, this PR, and the diagnostic alias repair. These are separate validation gaps, not attributed to a new regression here. Sharing the exact export settings or artifacts behind the reported YOLO results would help reconcile them.
Validation performed with Codex on Apple M1 Pro, macOS 26.6.2, PyTorch 2.14.0. Small models used seed 2026 and all-ones inputs; YOLO models used all-ones [1,3,640,640] inputs. The modified native sources were compiled from this PR's current head.
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.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
It changes low-level Metal buffer/view tracking and synchronization behavior in core runtime paths, which is correctness- and performance-critical and warrants final human review.
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.
Confirmed, fixed in a3acce5. Both same-address paths made a handle without taking a count on the view registration, while every deletion gave one back, so the first delete unregistered the view for the handle that was still alive.
metal_retain_view takes a count for an address that is a registered view and does nothing otherwise. aoti_torch_new_tensor_handle and the offset-0 branch of aoti_torch__reinterpret_tensor call it. I merged main to get the test_metal_memory target from #22954 and added four tests: both ways of making the second handle, in both deletion orders. Without the fix all four fail at the metal_is_device_pointer check after the first delete; with it test_metal_memory passes 7 of 7, and each test also checks the registration is gone once the last handle is deleted. Module suite: 134 run, 4 skipped as before; the only failures on my machine are the 10 int4 tests, which fail without this PR too (macOS 27.0 Metal compiler rejects the 4-bit shader source, BlockMMA / Atile). lintrunner is clean.
On the YOLO numbers: they came from exports with torch._inductor.config.layout_optimization = False, run on a build with #22952 applied as well. I should have said so, and I have corrected the description. Re-measured your way (all-ones [1,3,640,640] input, against eager, through executor_runner, this PR plus #22952):
model
layout_optimization=False
default
yolov8n
0.0019
crashes inside MPSGraph, called from the convolution
yolo11n
0.0020
same bmm layout failure you saw ([2,64,400], strides [1,800,2])
The yolov8n crash happens on macOS 27.0 with and without today's changes, so I could not reproduce your 357 figure. For default layout I would expect the C2f blocks to hit the limitation already listed under "Not fixed here": chunking along C in channels-last gives non-packed views, and writes through those are lost (linear_chunk_cat_last_dim).
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🔵 Needs a closer look
It changes core Metal runtime pointer/buffer resolution and synchronization behavior in multiple hot paths (binding, copies, MPSGraph interop), where subtle lifetime/hazard edge cases warrant final human review.
Follow-up for the "Not fixed here" item: #22984 keeps non-packed views in their parent's buffer (stacked on this PR), with #22983 as the issue. It turns out the materialized copy was also indexed out of bounds by the generated kernels, which is what crashed yolov8n under the default layout; with both PRs yolov8n runs correctly with layout optimization on.
The following ciflow label(s) have been added but CI has not been triggered yet because the workflows are awaiting approval:
ciflow/trunk
Once a maintainer approves the workflows (scroll to the bottom of the PR page), the corresponding CI jobs will be triggered automatically. Please ping one of the reviewers if you do not have access to approve and run workflows.
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed 89dbe34077f8f8dfb7d91c882c75d993096ffd2f, including all discussion, inline comments, and submitted reviews. The earlier copied-handle / zero-offset-alias registration finding is fixed: the numerical repros pass again.
The core fix also holds up: all 22 existing native checks and all 20 supported exported model/dtype cases pass, including 12 fresh split/unbind/narrow/slice/concatenation cases with nonuniform inputs. Additional successful native checks cover 120 nested-alias deletion orders, repeated offset reuse, and offset operands in mm/addmm/bmm/topk. The two documented non-packed-write negative controls remain unchanged and belong to #22984.
I would address the graph-specific synchronization requirement before approving; the inline comment includes a controlled reproduction where another graph consumes the pending wait. I also found a CPU-backed-view behavior regression at the native shim level and would preserve that behavior or explicitly settle whether it is supported. I have not reproduced the CPU case through an exported model.
Validation performed with Codex on Apple M1 Pro, macOS 26.6.2, PyTorch 2.14.0, with the changed runtime files rebuilt separately from base and this head. The original non-packed-read model fails on base and passes here. The issue's macOS 27 standalone alias hazard still does not reproduce on this machine, so that platform-specific result remains author-supplied.
The reason will be displayed to describe this comment to others. Learn more.
The wait can be consumed by a different graph from the one whose offset buffers armed it. I reproduced this with two threads and promises imposing the following order, without concurrent access to the allocation maps:
A calls get_mtl_buffer for an offset input, which arms syncAfterNextGraph_.
B executes an unrelated ordinary mm using base buffers. B consumes the flag.
A executes its prepared graph. It returns with a pending command buffer, without the wait required for its alias.
This demonstrates the missing synchronization at the native graph API level. The output was correct after an explicit final wait on this M1 Pro/macOS 26 machine; I am not claiming an independently reproduced concurrent-model numerical failure. This is the association problem behind the earlier review's shared-flag concern. Making the bool atomic would not fix it.
There is a single-thread error-recovery case too: mm obtains its offset first input successfully, then fails to resolve an unmapped second input. The flag survives and makes the next ordinary mm commit and wait. I reproduced this on the PR; the base leaves the next ordinary graph pending as usual. Moving the setter after successful alias allocation fixes that allocation-failure case, but a caller can still fail while collecting its other buffers or preparing the graph.
Please carry the synchronization requirement with the specific graph invocation, with the necessary encode/wait sequence serialized appropriately, so another invocation cannot consume it and failed preparation cannot leave it pending.
Complete native two-thread reproduction
Built as Objective-C++ against the PR runtime and the Foundation/Metal/MPSGraph frameworks.
The reason will be displayed to describe this comment to others. Learn more.
Agreed, the flag had no tie to the graph that armed it. Fixed in 67d8d5d: the stream keeps no such state any more. get_mtl_buffer only reports through an out-parameter that it made an alias; the op passes that to its own executeMPSGraph(..., settle_aliases), which waits before and after encoding that graph inside the same dispatch_sync block on the stream's serial queue, so no other graph or kernel can be encoded in between and nothing is left behind if the op fails before reaching its graph. syncAfterNextGraph is removed.
Tests in test_metal_memory: FailedAliasedGraphLeavesNoWaitBehind is your single-thread case (offset self, unmapped mat2, then an ordinary mm must be left pending); it fails on 89dbe34 and passes now. AliasedGraphSettlesItsOwnWork checks the offset graph's result is readable as soon as mm returns. I did not add the two-thread interleaving as a test; with the requirement carried by the call and serialized with the encode, there is no shared state left for another invocation to consume.
The reason will be displayed to describe this comment to others. Learn more.
This drops the previous no-copy mapping for a packed offset view whose parent is CPU-backed. Two native cases pass on the base and fail on this head:
Allocate eight float32 elements with device_type=0, reinterpret the last four as a 2x2 view, and use it as the input to aoti_torch_mps_mm_out with a Metal identity matrix and Metal output. The base returns [5,6,7,8]; the PR returns Error::Internal with self tensor not found in Metal buffer mapping.
Dispatch a Metal kernel writing [31,32,33,34] through that CPU-backed offset view. The base updates its parent; the PR leaves the parent unchanged. With no mapping, setArg uses setBytes, so the kernel writes a temporary.
A local diagnostic restoring the old metal_buffer_nocopy(adjusted_data, tensor->nbytes(), true) fallback only for an unmapped parent fixes both repros, while retaining this PR's parent-buffer registration for Metal-backed views and passing the other successful additional native checks.
Scope: these are native shim regressions, not an exported-model reproduction. The normal delegate execution path copies inputs to Metal allocations and prepares Metal outputs. Please preserve the previous CPU-backed-view behavior, or explicitly establish that this interface usage is unsupported before treating the behavior change as intentional.
Minimal MPSGraph-read regression test for runtime/test/test_memory.cpp
This uses the existing MetalMemoryTest fixture for platform initialization and cleanup. Add <algorithm> and the Metal stream header if needed, plus the declaration below.
The reason will be displayed to describe this comment to others. Learn more.
Right, that was an unintended behavior change. Fixed in 67d8d5d: an offset view of CPU memory gets its own no-copy buffer again. Unlike before, the buffer is counted with the view's handles and released with the last one (metal_register_cpu_view / metal_unregister_view), so it cannot outlive the CPU memory: with the ownership change below, that memory is now freed once its last handle goes. A longer view at the same address gets a buffer that covers it.
Your mm repro is in as CpuBackedOffsetViewIsUsableByMm, plus CpuBackedViewBufferGoesWithLastHandle and CpuBackedViewBufferCoversLongerViewAtSameAddress (the latter trips MPSNDArray ... buffer is not large enough without the length handling). The kernel-write case goes through the same mapping in setArg; test_metal_memory is plain C++, so I left that one to the mm read test.
…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.
Pushed 67d8d5d for both inline findings (replies on each). One more change in the same commit, found while checking the memory accounting around them: 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. So the parent was either never freed, or, after deleting the base and the first view, freed while a nested or copied handle still pointed into it. Every handle into memory the runtime owns now holds a count on the allocation it lives in and gives it back when deleted (ParentFreedAfter*, NestedViewKeepsParentAlive, CopiedViewHandleKeepsParentAlive; all fail on 89dbe34). This was previously only in #22984.
test_metal_memory 16/16; module suite 134 run, 4 skipped, the only failures being the 10 int4 tests, which fail on macOS 27.0 without #22982 (merged, not in this branch). lintrunner clean.
…-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.
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.
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
ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.community: contributionPRs coming from community (excluding hardware partners)module: metalIssues related to the AOTI Metal backend
5 participants
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.
meta-cla
Bot
added
the
CLA Signed
powerofaisinstudy-debug
left a comment
mergennachin
left a comment
executorch-triage
Bot
added
the
community: contribution
Summary
Fixes #22956
Ops consuming or filling a view that starts partway into a Metal buffer got stale data, silently. Anything built on
chunk/split/catis affected; the YOLO family (C2f blocks) ran but returned box coordinates off by hundreds.Packed views with a storage offset.
aoti_torch__reinterpret_tensorgave each one its own no-copyMTLBufferover the parent's memory (metal_buffer_nocopy). Metal treats the two buffers as unrelated, and that is not just an ordering hint: a write through the alias followed by a read of the parent in the same serial compute encoder sees stale data 200 times out of 200 in a standalone Metal program, while binding the parent at an offset is correct 200 out of 200 (repro in the issue). Inductor does both directions: an op reads a chunk of a buffer another op just wrote, and the result of acatis filled by writing through views of it (buf6/7/8 = reinterpret_tensor_wrapper(buf9, ..., offset) // alias).Such a view is now registered against the buffer it lives in (
metal_register_view) and resolved to that buffer plus a byte offset (metal_resolve_buffer), which is whatETMetalKernelFunction::setArg,metal_copy_memoryandmetal_is_device_pointeruse. Registrations are counted, since several tensors can be views of one address, and released when the view tensor is deleted.MPSGraphTensorDatacannot address into a buffer, soget_mtl_buffer(mm, addmm, bmm, convolution, topk) still hands MPSGraph an alias for an offset view, but now synchronizes the stream before creating it and has the stream wait again after the graph runs, so the memory is settled on both sides. On macOS 15+MPSNDArray initWithBuffer:offset:descriptor:would remove the need for that; left for a follow-up.Non-packed views are copied on the CPU by
materialize_packed, which waits for pending GPU work only ifmetal_is_device_pointer(src).srcwas the offset pointer, which was never registered, so the wait was skipped for every non-zero offset. The caller now says where the owning buffer lives, decided from the base pointer.Not fixed here: writes through a non-packed view are still lost, because the view is materialized into a copy.
linear_chunk_cat_last_dimcovers it and is registered with askipreason. It needs the materialization to move out ofreinterpret_tensor(generated kernels index by stride and only need the right base address; it is the MPSGraph ops that need dense input), which is a larger change I would rather discuss first.Test plan
Five modules added to
MODULE_REGISTRYinbackends/apple/metal/tests/test_modules.py, each in float32 and bfloat16:linear_chunk_last_dim(read of a non-packed view),linear_chunk_first_dim(read of a packed view),linear_chunk_cat_first_dim(writes through packed views, then a read of the parent),linear_nested_chunk(a chunk of a chunk, which inductor flattens into a single offset view of the base buffer),linear_chunk_cat_last_dim(skipped, see above).main: thelinear_chunk_*output tests fail withmax_rtol=1.0(the consumer read zeros).last_dimread passes, the rest still fail, which is what separated the causes.Ran 130 tests ... OK (skipped=4)for the full suite beforelinear_nested_chunkwas added; its 4 tests were then run on their own and pass.torch._inductor.config.layout_optimization = False:yolo11n,yolov8nandyolov5nwent from a max abs difference of 340 to 515 against their XNNPACK exports (same input, values up to ~640) to 0.002. Against eager on an all-ones input throughexecutor_runner:yolov8n0.0019,yolo11n0.0020. With inductor's default layout optimization these models still do not run correctly (unsupportedbmminput layout inyolo11n, and the non-packed write-through limitation above).lintrunnerclean on the touched files.