Skip to content

Copy returned views out of AOTI methods as contiguous tensors - #23894

Open
edmundshieh wants to merge 2 commits into
pytorch:mainfrom
edmundshieh:aoti-contiguous-returned-views
Open

edmundshieh wants to merge 2 commits into
pytorch:mainfrom
edmundshieh:aoti-contiguous-returned-views

Conversation

@edmundshieh

@edmundshieh edmundshieh commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

ReplaceViewCopyWithViewPass turns view copies such as slice_copy into views, so that AOTInductor compiles them natively. When the view is what a method returns, AOTInductor returns it with its base's strides. Since #24222 the CUDA backend accepts such an output, but it copies it through host memory with a stream sync on every call, and it turns off CUDA graphs for the method if they are enabled ("CUDA graph: disabled for '...', output N is not laid out like its ETensor"). A method that returns sample[..., :action_dim], as a robot policy's action head does, pays for this on every step.

The pass now clones a returned view with contiguous memory format, so that a kernel writes the output contiguously on the GPU and the method keeps its CUDA graph.

This PR was authored with AI (Claude Code).

Test plan

python -m pytest backends/cuda/passes/tests/test_replace_view_copy_with_view.py -v

test_returned_slice_is_cloned_contiguous and test_returned_select_is_cloned_contiguous fail without the change and pass with it. test_intermediate_slice_becomes_view checks that a view the method does not return is still not cloned. Run on Jetson AGX Thor against the 1.6.0.dev20261007 nightly with the pass from this branch: 3 passed, and 2 failed with the pass from main. backends/cuda/passes/tests/test_replace_fused_sdpa.py, which also runs the pass, passes with the change.

cc @nil-is-all @Gasoonjia @digantdesai

Copilot AI balanced review requested due to automatic review settings October 15, 2026 15:16
@pytorch-bot

pytorch-bot Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

🔗 Helpful Links

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

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

❌ 12 Unclassified Failures

As of commit 98963f5 with merge base 4411b8d (image):

UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:

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

@meta-cla 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 Oct 15, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: edmundshieh / name: edmundshieh (75e91e3)

@edmundshieh

Copy link
Copy Markdown
Contributor Author

@pytorchbot label "release notes: backends"

@edmundshieh

Copy link
Copy Markdown
Contributor Author

/easycla

@executorch-triage executorch-triage Bot added the community: contribution PRs coming from community (excluding hardware partners) label Oct 15, 2026
@nil-is-all nil-is-all added the module: cuda Issues related to the AOTI CUDA backend label Oct 16, 2026
@nil-is-all

Copy link
Copy Markdown
Contributor

Thanks for the PR @edmundshieh. Could you update the branch and ping when ready for review?

Copilot AI balanced review requested due to automatic review settings October 21, 2026 19:15
@edmundshieh
edmundshieh force-pushed the aoti-contiguous-returned-views branch from 75e91e3 to 3c60510 Compare October 21, 2026 19:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@edmundshieh

Copy link
Copy Markdown
Contributor Author

@nil-is-all Rebased onto the latest main, ready for review. The CI workflows are waiting for maintainer approval to run.

@Gasoonjia Gasoonjia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx LGTM!

Copilot AI balanced review requested due to automatic review settings October 21, 2026 20:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@edmundshieh

Copy link
Copy Markdown
Contributor Author

@nil-is-all Sorry, I updated the branch after the CI run was approved, which reset the workflow approval. Could you approve the runs again? On the previous commit (3c60510) everything passed except two jobs that also fail on main: Test WebGPU Backend (Dawn link error) and unittest-cuda-muse-glimmer-batching (CudaDFlashNative.FinalWholeBlockFitsExactlyAndPreservesAllPrefillRows, same assertion on main at 86b967e and 8d9eec3).

@Gasoonjia

Copy link
Copy Markdown
Contributor

hi @edmundshieh i've approved and retried the ci

@digantdesai

Copy link
Copy Markdown
Contributor

see "Raw Logs" on failed jobs here if expired - https://hud.pytorch.org/pr/pytorch/executorch/23894

@nil-is-all

Copy link
Copy Markdown
Contributor

@edmundshieh could you resolve CI failures? Do check the logs pasted in the above link

@edmundshieh
edmundshieh force-pushed the aoti-contiguous-returned-views branch from 30f5656 to ec0ffb3 Compare October 28, 2026 23:09
Copilot AI balanced review requested due to automatic review settings October 28, 2026 23:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@edmundshieh

Copy link
Copy Markdown
Contributor Author

@nil-is-all Thanks. The failures weren't from this change. On the run you approved, the PR's own tests passed (test_replace_view_copy_with_view.py, 3/3, in unittest-cuda, which was cancelled afterwards), and the failing jobs were breaking on main at the time: Test WebGPU Backend (Dawn link error), unittest-cuda-muse-glimmer-batching (CudaDFlashNative.FinalWholeBlockFitsExactlyAndPreservesAllPrefillRows), the muse-glimmer kquant e2e test (Hugging Face 404 for the tokenizer config) and the Windows CUDA wheel build (conda UnsatisfiableError). They all pass on PRs based on current main, so I rebased onto main (a96720a). Could you approve the CI run again?

@edmundshieh
edmundshieh force-pushed the aoti-contiguous-returned-views branch from ec0ffb3 to 0e60517 Compare October 29, 2026 14:47
Copilot AI balanced review requested due to automatic review settings October 29, 2026 14:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@edmundshieh

Copy link
Copy Markdown
Contributor Author

@nil-is-all On the last run everything passed except unittest-cuda-runtime-windows. It failed on two bugs in main at this PR's base (a96720a): setup.py called log.warning, which distutils doesn't have, and the job looked for a CUDA 13.0 toolkit the runner no longer has. #24238 fixed both shortly after, and the job has passed on main since. I rebased onto main (ffbc853). Could you approve the CI run again?

@nil-is-all

Copy link
Copy Markdown
Contributor

@nil-is-all On the last run everything passed except unittest-cuda-runtime-windows. It failed on two bugs in main at this PR's base (a96720a): setup.py called log.warning, which distutils doesn't have, and the job looked for a CUDA 13.0 toolkit the runner no longer has. #24238 fixed both shortly after, and the job has passed on main since. I rebased onto main (ffbc853). Could you approve the CI run again?

Thanks, triggered CI workflows. Do resolve merge conflicts while working on CI test logs, if any

ReplaceViewCopyWithViewPass turns view copies such as slice_copy into
views, so that AOTInductor compiles them natively. When the view is what
a method returns, AOTInductor returns it with its base's strides. The
CUDA backend then copies that output through host memory, with a stream
sync, on every call, and turns off CUDA graphs for the method if they
are enabled. A policy that returns sample[..., :action_dim] pays for
this on every step.

Clone a returned view with contiguous memory format, so that a kernel
writes the output contiguously on the GPU.
@edmundshieh
edmundshieh force-pushed the aoti-contiguous-returned-views branch from 0e60517 to 0365db7 Compare November 5, 2026 04:28
Copilot AI balanced review requested due to automatic review settings November 5, 2026 04:28
@edmundshieh

Copy link
Copy Markdown
Contributor Author

@nil-is-all Rebased onto main and resolved the conflict with #24222. The last run's only failure was an LLM serving test this PR doesn't touch; it passes on main. Since #24222 the runtime copies strided outputs itself, so I rewrote the description: the clone now keeps a returned slice contiguous on the GPU, so the method keeps its CUDA graph and skips a host round trip on every call. Could you approve the CI run again? @shoumikhin, does this still make sense alongside your runtime change?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

Copilot AI balanced review requested due to automatic review settings November 9, 2026 18:27
@shoumikhin

Copy link
Copy Markdown
Contributor

@claude review effort=max

@claude

claude Bot commented Nov 9, 2026 •

Copy link
Copy Markdown

Claude finished @shoumikhin's task in 24m 40s —— View job


PR Review: #23894

Request changes: the inserted edge.aten.clone is rejected by the Edge verifier in dim-order mode, so with the default EdgeCompileConfig any CUDA method that returns a slice, a select or a detach() now fails to lower; the change also stops two of #24222's CUDA-graph tests from ever seeing a strided output.

Detailed review

Summary

The PR makes ReplaceViewCopyWithViewPass clone a view that a method returns, with contiguous memory format, so the CUDA runtime gets a contiguous output and keeps its CUDA graph.

Problems found:

I couldn't run Python or the tests in this environment. Every finding below comes from reading the code.

Backward Compatibility

  • [Blocker] The inserted edge.aten.clone fails Edge verification in dim-order mode (backends/aoti/passes/replace_view_copy_with_view.py:92-96).
    • Why it fails: exir_ops.edge.aten.clone.default is a key of DimOrderOpsMap (exir/passes/dim_order_ops_registry.py:84). The Edge verifier raises SpecViolationError for every op in that map when _skip_dim_order=False (exir/verification/verifier.py:75-78, :332-333). This file's own comment at :35-38 says the same thing, and it is why detach_copy maps to alias_copy rather than to clone.
    • Where the check runs on CUDA:
      • preprocess runs this pass (backends/aoti/aoti_backend.py:323) and then the custom passes.
      • The custom passes always include MoveCondPredicateToCpuPass (backends/cuda/cuda_backend.py:1273).
      • That pass calls exported_program.validate() on every method, whether or not it has a cond (backends/cuda/passes/move_cond_predicate_to_cpu.py:94).
      • The partition program carries the owning Edge program's verifier (exir/lowered_backend_module.py:862).
    • Who hits it: anyone calling to_edge_transform_and_lower(export(M()), partitioner=[CudaPartitioner([])]) with the default EdgeCompileConfig() (exir/capture/_config.py:40-42), the call .ci/scripts/wheel/test_cuda_linux.py:420 makes. Such a call now raises ... should not be used in dim_order mode when the method returns a slice, a select or a detach(). Before this PR the same model lowered.
    • Why CI doesn't show it: the config the CUDA docs recommend (docs/source/backends/cuda/cuda-overview.md:49-52) turns the check off. So does backends/cuda/passes/tests/test_replace_fused_sdpa.py:33, which means its validate() at :109 checks nothing and its passing doesn't cover this change.
    • Suggested fix: insert alias_copy of the view instead of clone. That is what :39-40 already does for the same reason. Then assert the output's strides in a test, because the layout would come from Inductor's decomposition of alias_copy. Fix this →

Testing

  • [Major] Fix crashes, wrong outputs and lowering failures in several backends and in exir #24222's CUDA-graph tests stop producing a strided output, and nothing tests this PR's own claim end to end.
    • What changes: _SlicedLinear (backends/cuda/tests/test_output_stride_rearrange.py:277-283) returns self.proj(x)[:, :, :14], a returned slice_copy that this PR now clones. test_sliced_output_matches_eager_past_warmup and test_strided_output_first_seen_at_capture (:429-451) only compare values. They keep passing, but they no longer reach:
      • the warmup check that turns the graph off (backends/cuda/runtime/cuda_backend.cpp:1357-1365);
      • the copy through host memory (backends/cuda/runtime/utils.h:268-297).
    • The comment at :442-443 ("only the capture call sees a strided output") is no longer true. The PR's test plan didn't run this file.
    • Suggested fix:
      • For _SlicedLinear, assert that the runtime log contains "captured and instantiated for 'forward'", as :524 does. That tests this PR's claim on a GPU.
      • Keep the strided-output coverage with an output the pass leaves alone, such as the masked SDPA at :201-216.
    • Low confidence: _ConstantRow (:319-327) also returns a select that the pass converts. Please check that test_constant_copy_runs_without_a_graph (:553) still sees exactly one failed capture.
    • Fix this →
  • [Major] The new unit tests only check which ops are in the graph (backends/cuda/passes/tests/test_replace_view_copy_with_view.py:19-56).
    • The assertion targets[-1] == clone would still pass if memory_format were dropped, or if the clone did not feed the output.
    • Missing checks:
      • a validate() call on the program after the pass (with the default to_edge config at :21, this would have caught the verifier failure above);
      • running the transformed program and asserting that the returned tensor is contiguous and matches eager;
      • a case that returns a torch.split chunk, which is the only use of the getitem branch (replace_view_copy_with_view.py:80-84).
    • Fix this →

Performance

  • [Minor, low confidence] Metal also gets the clone, but the PR's reasons don't apply there.
    • MetalBackend inherits AotiBackend.preprocess (backends/apple/metal/metal_backend.py:21), so it runs this pass too.
    • Its runtime already copies every output with aoti_torch_copy_ (backends/apple/metal/runtime/metal_backend.cpp:602-611), and it has neither the copy through host memory nor CUDA graphs.
    • The description and tests cover CUDA only. Either check that the extra copy costs nothing on Metal, or apply the clone on CUDA only.

Code Quality

  • [Minor] A returned detach() gets a clone it doesn't need (backends/aoti/passes/replace_view_copy_with_view.py:63-69). detach_copy maps to alias_copy (:39-40), which is already a copy, not a view. But _returned_values runs for every node the pass converts, so return y.detach() becomes clone(alias_copy(y)). Only nodes that became views should get a copy.

Specific Comments

  • backends/aoti/passes/replace_view_copy_with_view.py:45: the class docstring, and the header at :7-10, still describe only the view replacement. They should also say that returned views are copied out contiguously.
  • backends/aoti/passes/replace_view_copy_with_view.py:66-67: "so a returned view is copied out as one" is unclear. Say what the copy avoids: the runtime copying the output through host memory and turning off the CUDA graph.

Recommendation

Request Changes

Tasks

I couldn't run the tests here: running Python is blocked in this environment.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. community: contribution PRs coming from community (excluding hardware partners) module: cuda Issues related to the AOTI CUDA backend release notes: backends

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants