feat: Re-inplace contiguous slice_copy as zero-copy memory.slice aliases (#10917) - #21552
feat: Re-inplace contiguous slice_copy as zero-copy memory.slice aliases (#10917)#21552iRAFEEK wants to merge 7 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21552
Note: Links to docs will display an error until the docs builds have been completed.
|
|
Hi @iRAFEEK! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
…torch#10917) Slice analog of ReplaceViewCopyWithViewPass. Detects contiguous (outermost-dim, unit-step) slice_copy nodes eligible to be re-inplaced as zero-copy slices. Rewrite is gated behind offset-based sub-buffer aliasing support in memory planning (pending design discussion), so the pass currently runs as a safe no-op.
Covers outermost-dim/unit-step eligibility, negative-dim resolution, strided/inner-dim rejection, and that the pass is a safe no-op until the offset-aliasing rewrite lands.
863138c to
a3fef4f
Compare
|
@pytorchbot label "release notes: none" |
|
@metascroy — CI workflows are awaiting maintainer approval to run. Could you approve them when you get a chance? Thanks! |
|
Thanks for the PR, @iRAFEEK. Running CI now. |
|
@nil-is-all — pushed a formatting fix (db374f2) for the lintrunner failure. lintrunner -a reports no issues locally, and the unit tests pass. Could you approve CI when you get a chance? Thanks! |
|
@nil-is-all @JacobSzwejbka could you please check it out now , thank you so much |
Sure, thanks. Running CI again |
|
@JacobSzwejbka — design check before I build out the runtime side of this. The blocker looks structural:
Two things I'd like a steer on:
Unless you object, I'll build it as described above and update this PR. I have the pass and memory-planning wiring prototyped locally and am filling in the correctness tests now (numerical parity against eager, plus negative cases for strided/inner-dim/channels-last bases falling back to copy). |
Replaces eligible contiguous slice_copy nodes with a memory.slice alias
so the emitted program does not pay for a full tensor copy.
_SliceSpec shares the base's mem_id and computes
mem_offset = base.mem_offset + start * base.stride[0] * elem_size.
The .pte format already carries (memory_id, memory_offset) via
AllocationDetails, so no schema change is required. Memory planning
handles memory.slice like memory.view -- the base spec is returned from
get_node_tensor_specs, which extends the base's lifetime over the
slice's consumers so the buffer is not reused while the alias is live.
Emission mirrors _emit_view's elide path, needing no runtime kernel.
Eligibility is gated to dim-0, unit-step slices with a non-negative
start on a base that has the default dim order and its own allocation.
Non-default layouts would otherwise be silently reinterpreted by the
contiguous output stride, and an aliasing base (slice-of-slice or
slice-of-view) has no concrete allocation to offset from. Everything
outside those gates falls back to slice_copy unchanged.
Also declares inplace_base on _SliceSpec, which the greedy memory
planning algorithm reads.
Verified locally against the executorch wheel runtime:
- contiguous slices emit no slice_copy kernel (only aten::add)
- outputs match eager for offset/lifetime/chained/3-D cases
- ineligible slices still fall back to copy and stay correct
- no regressions: exir/tests, exir/emit, exir/backend failure sets
are identical to a pristine baseline
f4a6896 to
a5fda43
Compare
|
@nil-is-all @JacobSzwejbka could you please check it out now? Thank you so much |
|
@nil-is-all @JacobSzwejbka Just following up if u can run the tests, please. |
Summary
A contiguous slice (e.g.
x[1:3]on a contiguous input) was always emitted as a full-copyaten::slice_copykernel, even though it can alias a sub-region of the base buffer — the same ideaReplaceViewCopyWithViewPassalready applies toview_copy. On a memory-constrained device that is a wasted allocation and a wasted copy on every inference.This implements #10917:
ReplaceSliceCopyWithSlicePassrewrites eligible contiguousslice_copynodes intomemory.slicealiases, so no copy kernel is emitted.How it works
_SliceSpecshares the base'smem_idand computesmem_offset = base.mem_offset + start * base.stride[0] * elem_size.AllocationDetailsalready carries(memory_id, memory_offset)andmem_offsetreaches it throughmake_allocation_info, so no schema or runtime change is needed._ViewSpeccan't be reused here because it requiresnbytes == base.nbytes()— a view aliases the whole buffer, a slice only part of it.memory.slicelikememory.view: it's on thecollect_specs_from_nodesskip-list, andget_node_tensor_specsreturns the base spec. Sinceupdate_all_tensors_lifetimewalkschain([node], node.args, ...), the base's lifetime is extended to cover the slice's consumers, so the buffer can't be reused while the alias is live._emit_view's elide path — static and memory-planned specs go straight through_emit_spec, so no runtime kernel is required.Eligibility. Restricted to dim-0,
step == 1, non-negativestart, on a base that has the defaultdim_orderand its own allocation. Non-default layouts would otherwise be silently reinterpreted by the contiguous output stride, and an aliasing base (slice-of-slice, slice-of-view) has no concrete allocation to offset from. Everything outside those gates falls back toslice_copyunchanged, so this is opt-in by construction.Per @JacobSzwejbka's note on the issue that other dim orders can be a follow-up, v1 keeps to the default layout.
Fixes #10917
Test plan
Copy elimination —
x[1:3] + 1.0lowered withto_edge(...).to_executorch():Unit tests (
exir/tests/test_replace_slice_copy_with_slice_pass.py, 9 tests) cover eligibility classification, negative-dim resolution, the rewrite itself, non-defaultdim_orderskipped, negativestartskipped, chained slices falling back, base-outlives-slice lifetime, and end-to-end parity against eager. The end-to-end test asserts the copy was elided, not just that the numbers match — a fallback to copy would pass a numerical check alone.Runtime verification against the executorch wheel, comparing
_load_for_executorch_from_bufferoutput to eager withtorch.arangeinputs so a wrong offset is visible:No regressions.
exir/tests,exir/emit, andexir/backend/testwere run against both this change and a pristine executorch install; the failure sets are identical (the pre-existing failures are missing quantized out-variants and backend runtime pieces in the wheel).exir/tests/test_memory_planning.py37 passed,exir/emit/test69 passed.lintrunner(including MYPY) is clean with no patch to apply.Checklist
dim_orderfalls back to copystartfalls back to copylintrunnercleanexir/passes/BUCK+exir/tests/targets.bzlregistration — happy to add once the approach is confirmedcc @JacobSzwejbka @metascroy