Fix device placement of constants, shared inputs and delegate outputs by shoumikhin · Pull Request #24088 · pytorch/executorch

shoumikhin · 2026-10-22T19:09:01Z

What is wrong today

When a delegate runs on a GPU, PropagateDevicePass copies its inputs to the device (_h2d_copy) and its outputs back (_d2h_copy). skip_h2d_for_method_inputs and skip_d2h_for_method_outputs remove those copies at the method's edges.

A transposed parameter is stored contiguous, but a delegate reading it is compiled for its original strides. to_edge does this silently.
When a host op writes a delegate output in place, the next delegate on that device reads the old value. Such a relu_ makes memory planning fail.
A lowered module returning one tensor, called from another module, gets no _d2h_copy.
With the output skip flag, a returned output that something else reads comes back on the host.
A constant that feeds a device delegate makes to_executorch fail. With the input skip flag, it arrives as host memory marked as device memory.
With the input skip flag, an input with more than one reader is refused.
A delegate targeting the CPU gets copies that fail at run time.

What this change does

Numbered as above:

to_edge makes such a parameter contiguous before partitioning, and warns for each tensor whose layout it changes.
The host op writes the host copy of the output. The copies stay when it writes between two delegates, also through a view.
The output gets its _d2h_copy.
Returned outputs stay on the device. One that a host op writes in place is returned on the host, with a warning.
The constant gets a device copy, with a warning. One neither contiguous nor channels-last is refused.
The input stays on the device of its first delegate. Shape-only ops read it there. Other readers share one _d2h_copy of it, with a warning. An input written in place on the host is refused.
The delegate gets no copies.

Also:

A tensor that several delegates on one device read is copied there once, unless written in between.
New option PropagateDeviceConfig(error_on_per_call_copies=True) turns the warnings in 4 to 6 into errors when the matching skip flag is set.

What was tested

Device pass tests and new exir/tests/test_passes.py tests pass. All new tests but one fail without their fix.
backends/cuda/tests/test_device_placement.py matches eager PyTorch with real CUDA delegates on an H100. The unittest-cuda job now runs when these passes change, and fails if that test skips.

Copilot

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

pytorch-bot · 2026-10-22T19:09:12Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

❌ 58 Pending, 5 Unclassified Failures

As of commit 964a611 with merge base 5fa57d1 ():

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:

trunk / test-models-macos-coreml (efficient_sam) / macos-job (gh) (this job did not run on the merge base, so DrCI cannot tell whether the failure is pre-existing)
executorch
trunk / test-models-macos-coreml (mv3) / macos-job (gh) (this job did not run on the merge base, so DrCI cannot tell whether the failure is pre-existing)
executorch
trunk / test-models-macos-cpu (efficient_sam, portable) / macos-job (gh) (this job did not run on the merge base, so DrCI cannot tell whether the failure is pre-existing)
executorch
trunk / test-models-macos-cpu (vit, xnnpack-quantization-delegation) / macos-job (gh) (this job did not run on the merge base, so DrCI cannot tell whether the failure is pre-existing)
executorch
trunk / test-static-llama-ane / macos-job (gh) (this job did not run on the merge base, so DrCI cannot tell whether the failure is pre-existing)
executorch

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

Copilot

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

Gasoonjia

Thanks for find and fixing the issue! I left some thoughts and we can discuss.

Gasoonjia · 2026-10-23T00:45:41Z

    new_spec = copy.copy(spec)
    new_spec.init_mem_planning_fields()
+    # A copy's output is planned memory, even when it copies a constant.
+    new_spec.const = False


i feel like it is a liitle bit dangerous if we overwrite the const attribute silently here. Maybe we can rename the function to highlight it, or create a new function. WDYT?

Good point. I renamed the helper to _copy_output_spec. Its docstring now says the output is always planned memory, so it is never a constant, even when the tensor it copies is one. It is only used to build the spec of an _h2d_copy or _d2h_copy output, so no other spec gets its const flag changed.

Gasoonjia · 2026-10-23T00:51:10Z

+            if (
+                self.skip_h2d_for_method_inputs
+                and self._is_placeholder(arg)
+                and not arg_spec.const


im ok with this change to fix the correctness isssue but from api design perspective it doesn't quite in line with skip_h2d_for_method_inputs: constant var is also pert of method inputs (although implictly), and if user turn it on but still need to do h2d copy for constant it is not elegant.

My understanding is we may need to have devcie-resident constant materialization so that we can treat contant as regular input, in the device support scenerio.

I agree that a constant is a kind of method input too. The problem is that today the runtime always points a constant at its data in host memory (in the program or in the external data file), whatever device its spec says. So if we skip the copy, the delegate gets a host pointer that is marked as device memory. Until the runtime can load constants into device memory, we need the copy to stay correct. I added a line to the comment on skip_h2d_for_method_inputs that says constants are still copied. Device-resident constants sound like a good follow-up, and then this flag could skip their copy too.

This is now in #24144, which builds on this PR. With skip_h2d_for_method_inputs, a constant that only delegates on one device read is loaded into device memory once, when the method loads, so it needs no copy per call.

Copilot

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

Copilot

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

Copilot

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

Copilot

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

A partitioner can set the target_device compile spec to "cpu". The pass treated that delegate like one on a device: it copied each input with _h2d_copy and each output with _d2h_copy, both to and from the CPU. The copy kernels refuse a CPU destination for _h2d_copy and a CPU source for _d2h_copy, so such a program failed at run time. A delegate on the CPU reads and writes host memory directly, so the pass now leaves it alone, the same as a delegate without a target device. The new test lowers an add to a delegate with target device "cpu" and expects no copies. It fails without this change.

With skip_h2d_for_method_inputs, the caller passes an input that a device delegate reads in device memory. Until now, to_executorch refused such an input when anything else read it too: a host op, a delegate on the host, or a delegate on another device. The pass now keeps the input on the device for the delegates there and gives every other reader one _d2h_copy of it, the same way it already handles a device delegate output read on the host. A delegate on another device then reads a copy of that host copy. The input stays on the device of the first delegate that reads it, in graph order, and later delegates on that device find it already placed. An op that reads only the input's shape, such as sym_size, reads it on the device, since sizes stay in host memory. The copy runs on every call, so the pass logs a warning with the input's size and a reader. With skip_d2h_for_method_outputs, a method that returns the input returns it on the device, with no copy. An input that an op writes in place on the host is still refused, now with its own message, because the write would only reach the host copy. The refusal test becomes a test that the input stays on the device and that the other readers read one D2H copy, with the warning, for a host op, two delegates on the device plus a host op, a delegate without a device, a delegate on the CPU, a delegate on another device and a buffer write-back. It fails without this change. New tests return an input with both skip flags, and read only the shape of an input on the host, and expect no copy and no warning. A new refusal case covers a host op that writes the input in place, and a new CUDA test runs a program whose input on the device is also read on the host and compares with eager PyTorch.

With skip_h2d_for_method_inputs and skip_d2h_for_method_outputs, users expect no copies at the method boundary. Three copies still stay and run on every call, and to_executorch only logs a warning for them: a constant that feeds a device delegate from outside it, a method input on the device that a reader off that device also reads, and a returned output that a host op writes in place, which is copied to the host and back. A warning is easy to miss in a long export log. The new opt-in PropagateDeviceConfig option error_on_per_call_copies makes to_executorch raise with the same message instead. It applies to a constant copy only with skip_h2d_for_method_inputs, since without it every input is copied anyway. The input copy exists only with that flag, and the written output copy only with skip_d2h_for_method_outputs. The messages now format sizes with an f-string, as the error and the warning share them. The new test expects the error for all three cases, and only the warning for a constant copy without the skip flag. It fails without this change.

The notes this change adds to the CUDA backend page, about the copies that stay with skip_h2d_for_method_inputs and skip_d2h_for_method_outputs, were long paragraphs that explained the internals. They are now a short list of the copies that stay, how to remove them, how to make them an error, and which inputs the skip flag refuses. They also move after the other notes on the Python export options.

Copilot

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

shoumikhin · 2026-10-26T16:00:30Z

@claude review

claude · 2026-10-26T16:01:27Z

Claude finished @shoumikhin's task in 6m 9s —— View job

I found one thing worth checking before merge, at low confidence: the new parameter rewrite in convert_constant_dim_order_pass may write into the caller's ExportedProgram, so a second to_edge on the same program could pair contiguous data with the old strides. There are also two minor CI and test gaps.

Detailed review

PR Review: #24088

Summary

The PR fixes where PropagateDevicePass places constants, shared method inputs and delegate outputs, adds error_on_per_call_copies, and makes to_edge store non-contiguous parameters contiguous. There are no blockers. One backward-compatibility risk is suspected but not confirmed, and there are two minor gaps.

Coverage:

Three independent reviewers covered all 10 files, and I re-read the code behind each finding below.
Nobody ran any code: Python and git fetch were blocked in this run.
exir/tests/test_propagate_device_pass.py (about 1,170 added lines) was only partly read.
I left out points the earlier reviews raised that the author already answered: the alias through getitem, the constant warning without a skip flag, the private helper imports, the position map, and _skip_dim_order in the CUDA tests.

Backward Compatibility

exir/passes/convert_constant_dim_order_pass.py:101-106: the parameter rewrite may change the caller's program (suspected, low confidence).
- What it does: it assigns into exported_program.state_dict[key]. If run_decompositions passes ep.state_dict through to the new program as the same dict object (and remove_unused_parameters_pass already deletes from ep._state_dict in place on that assumption), this replaces the tensor in the user's own ExportedProgram.
- Result: after the first to_edge(ep), ep.state_dict["weight"] is contiguous, while p_weight.meta["val"] in the user's graph still has the transposed strides.
- Trigger: export a module whose parameter is nn.Parameter(torch.randn(5, 4).t()), then call to_edge(ep) twice, or to_edge_transform_and_lower(ep) after a to_edge(ep). A comparison or ETRecord flow can do this.
- Second run: decomposition copies the strided placeholder meta from the user's graph. _should_transform(param) then looks at the stored tensor, which is already contiguous, and returns False, so the meta is never fixed. The stored data and the strides the partitioner and delegate see then disagree.
- Scope: constants and buffers already had this problem before the PR. Parameters are new to it, and far more common.
- Suggested fix: build a new dict instead of writing into the shared one, or decide from placeholder_node.meta["val"] as well as the stored tensor.
- Tests: test_convert_constant_dim_order_parameter passes copy.deepcopy(ep), which would hide this. A test that lowers the same ep twice, or checks that the input ep is unchanged, would settle it.
Fix this →

Infrastructure

.github/workflows/cuda.yml:489-492: a change only to the device passes also runs the whole kernel shard.
- The four new path checks are in the job-level if: of unittest-cuda, which is a 2-shard matrix.
- test_device_placement.py runs only in the system shard.
- So a PR that only changes propagate_device_pass.py also gets the full CUDA build and every Triton kernel test on a GPU runner, none of which test the change.
- A job-level if: cannot read matrix. The fix is an early exit in the kernel script when only these paths changed, or a small separate job. (I can't edit workflow files from this action.)

Testing

backends/cuda/tests/test_device_placement.py:125-147: test_input_on_device_also_read_on_host only compares outputs with eager.
- It never checks that the input stayed on the device and that the host op read a _d2h_copy, the path the test is named after.
- Wrapping load in assertLogs(propagate_device_pass.logger, "WARNING"), or counting the copy nodes, would tie the test to that path at no extra cost.

Recommendation

Approve

Nothing blocks merging on the evidence here. Please check the shared state_dict question with a quick double-to_edge test. If the dict is shared, fix it before merge, because the rewrite now runs for every backend's to_edge.

shoumikhin · 2026-10-26T16:16:17Z

Thanks. No code change here:

Shared state_dict: both to_edge paths run the pass only on the program run_decompositions returns, which has its own state_dict. So the caller's program keeps its strided parameter. I checked it: export a transposed parameter, change it the way the pass does on the decomposed program, and decompose the original again. Its data and graph are still strided.
kernel shard: true. The pybindings paths already start both shards the same way. Giving each shard its own paths would be a separate workflow change.
test_input_on_device_also_read_on_host: test_skip_h2d_copies_input_for_readers_off_device already checks that the input stays on the device, that the host op reads one _d2h_copy, and the warning. The CUDA test checks the GPU result.

With skip_d2h_for_method_outputs, the method returns delegate outputs on the device. When a custom host op with a mutable argument, such as a KV cache update, writes the host copy of such an output in place, the device tensor never sees the write. The pass used to copy the written host tensor back to the device and return that copy, so every call paid for a copy to the host and another one back. The written host tensor is already the right result, so the method now returns it as it is, and the method output says it is in host memory. The copy to the host stays, because the host op needs it. The warning about it stays too, and error_on_per_call_copies still turns it into an error.

shoumikhin · 2026-10-26T18:55:25Z

@claude review

Copilot

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

claude · 2026-10-26T18:56:15Z

Claude finished @shoumikhin's task in 6m 25s —— View job

I found no blockers, only minor issues: the docs and config comment say that only a custom host op sends a returned output back to the host, but the code does this for any op that writes it in place, and the per-call copy warnings for skip_h2d_for_method_inputs inputs give wrong advice or understate the copies in two cases.

Detailed review

PR Review: #24088

Summary

The PR fixes where PropagateDevicePass places constants, shared method inputs and delegate outputs, adds error_on_per_call_copies, and makes to_edge store non-contiguous parameters contiguous. Nothing blocks merging. The findings below are small doc, API and diagnostic gaps, plus two suspected edge cases in how the pass detects writes.

Coverage:

Three independent reviewers covered all 10 files, and I re-read the code behind each finding below.
Nobody ran any tests.
exir/tests/test_propagate_device_pass.py was only partly read.
Points the author already answered in earlier rounds are left out.

API Design

exir/passes/propagate_device_config.py (the skip_d2h_for_method_outputs comment) and docs/source/backends/cuda/cuda-overview.md (third bullet of "Some copies stay"): both say a returned output goes back to the host only when a custom op with a mutable argument writes it in place. Two reviewers found this.
- What the code does: _keep_method_outputs_on_device (propagate_device_pass.py:608) uses _in_place_writers. That counts any op whose schema writes the tensor, including aten in-place ops.
- Trigger: run_reinplace_pass=True or a user pass in config.passes leaves index_put_ or add_ on the host copy of a returned delegate output. That output comes back on the host, and with error_on_per_call_copies=True export fails. The docs give no hint that this can happen.
- Fix: say "an op outside the delegate that writes it in place" in both places.
exir/passes/propagate_device_pass.py:330-339: error_on_per_call_copies=True is accepted even when it can have no effect.
- Why: _report_per_call_copy raises only when skip_requested is set. Every call site passes a skip flag, or True inside a path that only runs under skip_h2d/skip_d2h.
- Trigger: PropagateDeviceConfig(error_on_per_call_copies=True) with no skip flag set. The constant copies still run on every call and only log warnings, and the user who wanted that guarantee gets no error.
- Fix: raise a ValueError like the existing check for enable_non_cpu_memory_planning does, or document that the flag does nothing without a skip flag.

Code Quality

exir/passes/propagate_device_pass.py:498-516: in two cases, the per-call copy message for a skip_h2d input says the wrong thing.
- Case 1, returned input: skip_h2d_for_method_inputs=True is set and skip_d2h_for_method_outputs is not, and the method returns an input directly, as in return x + y, x with the add on CUDA. The output node then counts as another reader. The message says "Have the partitioner put every reader in a delegate", which can't be done for the method output; the real fix is skip_d2h_for_method_outputs. With error_on_per_call_copies=True this raises, although neither the docs bullet ("an operator outside the delegate or a delegate on another device") nor the config comment lists the output as a case. Either name the output in the message and the docs, or don't count it as a per-call copy that the skip flag asked to remove.
- Case 2, delegate on another device: the cuda:1 delegate reads _h2d_copy(_d2h_copy(x)), which is two copies per call. The message says only "copied to the host on every call". The second _h2d_copy is made silently in _insert_h2d_copies, because its argument is no longer a placeholder or a constant, so error_on_per_call_copies never sees that half.
exir/passes/propagate_device_pass.py:600-606: the output check only looks at _d2h_copy nodes that the output reads directly (suspected).
- Trigger: a returned delegate output y is written in place on the host as r = add_(d2h(y), 1), and the method returns r instead of the copy itself.
- Result: the output stays on the host. No warning is logged, and error_on_per_call_copies does not raise, although the docs promise it fails for such an output. Checking whether the output reads any alias of the copy would cover this case.
exir/passes/propagate_device_pass.py:185-215, :256: _written_between(tensor, copy, delegate) sees writes to tensor and to views of it, but not writes to the tensor that tensor is a view of (suspected, low confidence).
- Trigger: a delegate on cuda:0 reads t = as_strided(base), then a host op writes base in place, then a second delegate on cuda:0 reads t.
- Result: the second delegate reuses the first _h2d_copy of t and reads stale data.
- Reachability: I could not show that unsafe_remove_auto_functionalized_pass or reinplacing actually produces this graph. The docstring's view handling only covers the other direction, so it is worth either a note that this case can't happen or a check for it.

Testing

exir/tests/test_passes.py:3198: the comment "to_edge runs the pass twice, but only the first run converts" is wrong for the _skip_dim_order=True subtest. In that mode _program.py:826 skips the first call, so the pass runs once, as the new comment at _program.py:876-879 says. The len(captured.output) == 1 assertion passes, but for a different reason than the comment gives.

Recommendation

Approve

None of these block merging. The doc and message fixes are one-line changes. The two suspected detection gaps are worth a quick check, or a comment explaining why they can't happen.

The docs and the config comment said that a returned output goes back to the host only when a custom op writes it in place. The pass does this for any op outside the delegate that writes it, aten in-place ops included, so the text now says that. The warning for a method input that stays on the device was also off in two cases. When the method returns the input, the advice to put every reader in a delegate cannot work, and skip_d2h_for_method_outputs is what removes the copy. When a delegate on another device reads the input, it gets a second copy, from the host to its device, which the warning did not mention. The warning, the config comment and the docs now cover both cases.

With _skip_dim_order, to_edge runs convert_constant_dim_order_pass once, not twice, so the comment now covers both modes.

Copilot

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

shoumikhin · 2026-10-26T20:08:26Z

Thanks. Addressed in the latest push:

Docs and config comment: any op outside the delegate that writes a returned output in place sends it back to the host, not only a custom op. Both now say so.
Input copy warning: it now says the copy also goes on to any other device that reads the input, and suggests skip_d2h_for_method_outputs when the method returns it. The docs list both cases.
Fixed the to_edge test comment.

error_on_per_call_copies without a skip flag: the docs say it fails only on copies that a skip flag you set should remove.

@claude review

claude · 2026-10-26T20:09:22Z

Claude finished @shoumikhin's task in 6m 19s —— View job

I found no blockers in the latest push. The three fixes you listed are in. Four small issues remain: the docs give a KV cache update as the example for the wrong case, the output warning doesn't name the output or say it needs a host buffer, and two tests are weaker than they look.

Detailed review

PR Review: #24088

Summary

The PR fixes where PropagateDevicePass places constants, shared method inputs and delegate outputs, adds error_on_per_call_copies, and makes to_edge store non-contiguous parameters contiguous. I found nothing that blocks merging, only the minor doc, message and test issues below.

Coverage:

Three independent reviewers covered all 10 files, and I re-read the code behind each finding below.
Nobody ran any tests.
About half of exir/tests/test_propagate_device_pass.py was only partly read.
Points that earlier rounds already settled are left out.

API Design

exir/passes/propagate_device_config.py:70-71 and docs/source/backends/cuda/cuda-overview.md:221: a KV cache update is the wrong example for "an op that writes a returned output in place".
- A KV cache update writes the cache. The cache is a mutable buffer, so it is a placeholder, not a delegate output.
- That case goes through the buffer write-back. With skip_h2d_for_method_inputs, it is the case refused a few lines later (cuda-overview.md:230), which itself uses "a mutable buffer, such as a KV cache" as its example.
- So the same doc sends KV cache users to two different rules.
- Fix: use an example that does write a delegate output, such as a mutable custom op left on the host, or the add_ left by reinplacing.

Code Quality

exir/passes/propagate_device_pass.py:612-621: the warning for a returned output written on the host leaves out two things the caller needs.
- Which output: it says only "A method output (N bytes)". The input and constant warnings both name their node. If a method returns several outputs that host ops write, the warnings differ only in byte count. Include returned.name or the output index.
- Host buffer needed: the config docstring says to pair skip_d2h_for_method_outputs with MemoryPlanningPass(alloc_graph_output=False). Under that pairing this output is not memory planned, so the caller supplies its buffer, and the _d2h_copy and the host op both write into it.
- A caller who follows the "outputs stay on the device" contract passes a device buffer, and host kernels then write through a device pointer.
- cuda-overview.md:222-223 mentions the method metadata. The warning only says "returned there". Saying "the caller must pass a host buffer for this output" would make the contract clear where it actually comes up.

Testing

exir/tests/test_propagate_device_pass.py:981 (test_skip_d2h_returns_output_written_on_host_from_host) does not test the documented pairing.
- It sets no alloc_graph_output=False, so it never checks that this output ends up unplanned.
- It never checks that the emitted method metadata marks the output as CPU, which is the only signal a native caller gets.
- backends/cuda/tests/test_device_placement.py has no skip_d2h_for_method_outputs case, so a mix of host and device outputs is never run on a GPU.
Two asserts cannot fail:
- test_propagate_device_pass.py:2270: assertFalse(set(copy.users) & set(nodes.d2h_nodes)) always holds, because the pass never feeds one _d2h_copy into another. In the "two delegates on the device and a host op" case, nothing checks that the second cuda:0 delegate reads a directly, and that is the claim in the docstring.
- test_constant_copy_keeps_stored_layout (:1975), channels-last subcase: _copy_output_spec copies the constant's spec, and the emitter rewrites only layouts that are neither contiguous nor channels-last, so the two specs are equal by construction. Only the transposed subcase can fail.

Recommendation

Approve

None of these block merging. The doc example and warning text are one-line changes. The test gaps are worth closing, so that the host-output path under alloc_graph_output=False is checked at least at graph and metadata level.

Some PyTorch builds turn off auto_functionalized v2 for export. There, a custom op that writes through a view writes into a view_copy, not into an as_strided of the tensor, so the tests that write through a view see a different graph and fail. Under v1 that write is lost even without these passes, so there is nothing correct to check there. Functionalize with v2 in these tests so they check the same graph in every build.

The warning for a returned output that an op writes on the host now says which output it is and that a caller passing its own output buffers must pass a host buffer for it. The docs and the config comment now give a mutable custom op left on the host as the example, since a KV cache update writes a buffer, which is a different case.

Copilot

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

shoumikhin · 2026-10-26T20:57:08Z

Thanks. In the latest push:

The output warning now says which output it is, and that a caller passing its own output buffers must pass a host buffer for it.
The docs and config example is now a mutable custom op, not a KV cache update.
The host writer tests pin auto_functionalized v2, so they check the same graph in builds that export with v1.

The test gaps (no unplanned host output case, two asserts that cannot fail) are in older tests and do not hide a bug here. Better tightened separately.

@claude review

claude · 2026-10-26T20:57:57Z

Claude finished @shoumikhin's task in 6m 10s —— View job

I found no blockers in the latest push, and the three changes you listed are in; four minor issues remain: one docs sentence, two test gaps from this round, and one alias question from an earlier round that hasn't been answered yet.

Detailed review

PR Review: #24088

Summary

The PR fixes where PropagateDevicePass places constants, shared method inputs and delegate outputs. It also adds error_on_per_call_copies, and to_edge now stores non-contiguous parameters contiguous. Nothing blocks merging; the findings below are minor.

Coverage:

Three independent reviewers covered all 10 files, and I re-read the code behind each finding below.
Nobody ran any tests, and git fetch was blocked.
The checkout is shallow, so I diffed the 10 PR files against origin/main. The totals match the PR's +1955/-97.
exir/tests/test_propagate_device_pass.py was only partly read.
I left out points settled in earlier rounds.

API Design

docs/source/backends/cuda/cuda-overview.md:226: this sentence contradicts the list just above it.
- The sentence says error_on_per_call_copies "fails only on a copy that one of the skip flags you set was meant to remove".
- But the same section (lines 211-220) lists the constant copy as one that stays even with the skip flags. The config comment also says that skip_h2d_for_method_inputs does not remove constant copies.
- In the code, the flag does raise on the constant copy when skip_h2d_for_method_inputs is set (propagate_device_pass.py:417-424).
- So a reader of the docs would not expect export to fail on a weight that a delegate reads from outside.
- propagate_device_config.py:82-88 describes this correctly ("the copies that the skip flags above cannot remove"). Using that wording, or naming the three cases, would fix the docs.

Testing

exir/tests/test_propagate_device_pass.py:1974-1978: test_error_on_per_call_copies doesn't check which copy raised.
- All three subtests use the same regex, "on every call.*Raised because error_on_per_call_copies=True", and that text appears in the constant, input and output messages alike.
- So a regression where the wrong check raises in a case would still pass. Two examples: the constant check firing in "input read on host", or the input check firing in "output written on host".
- Asserting each case's own prefix would close this: Constant 'b_offset', Method input 'x', Method output 0.
exir/tests/test_propagate_device_pass.py:2381-2414: one host writer case isn't pinned to auto_functionalized v2.
- The InputWrittenByHostOp case in test_skip_h2d_refuses_placeholder_written_on_host calls propagate_device_test.accumulate.
- It lowers with plain to_edge(export(...)) instead of _lower_with_host_writer, so it doesn't get the pin that 4c56ae2 adds to the other host writer tests.
- The write is direct, not through a view, so v1 probably builds the same graph (suspected, not run). Routing it through _lower_with_host_writer would make it match the other host writer tests.

Code Quality

exir/passes/propagate_device_pass.py:185-215: _in_place_writers only follows aliases forward, from a tensor to views made from it (suspected, low confidence).
- Two reviewers raised this again this round. It was raised in an earlier round too, and has no reply yet.
- What it misses: if the tensor a delegate reads is itself an alias, such as an as_strided view or the result of an in-place op on z, a later host write to z, or to another view of z, is not found.
- Result: _reusable_copy (line 256) and the round-trip elision would then reuse a stale copy.
- Reachability: it's probably unreachable today. reinplace refuses when the base is used later, and auto_functionalized removal makes its views only for the writer's arguments.
- Ask: add a line to the docstring saying why the reverse direction can't happen, or check for it.

Recommendation

Approve

None of these block merging. The docs fix is one sentence, and the test changes are small.

shoumikhin · 2026-10-26T21:06:03Z

Thanks. Answers to the rest, without another push:

The error_on_per_call_copies docs sentence is loose for constants, which do raise under skip_h2d_for_method_inputs. The config comment has the exact list; I will align the docs with it separately.
The direct-write refusal case passes with v1 too, since no view is written.
_in_place_writers only needs the forward direction here: reinplacing refuses when the base is used later, and removing auto_functionalized only makes views of the writer's own arguments.
Asserting each case's own prefix in test_error_on_per_call_copies is a fair test-only tightening for later.

shoumikhin requested review from JacobSzwejbka and larryliu0820 as code owners October 22, 2026 19:09

Copilot AI balanced review requested due to automatic review settings October 22, 2026 19:09

shoumikhin added the release notes: exir Changes to any dialects and passes on these dialects, such as memory planning label Oct 22, 2026

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 22, 2026

Copilot AI reviewed Oct 22, 2026

shoumikhin force-pushed the device-pass-constants branch 2 times, most recently from 3aadad6 to 894b0fa Compare October 22, 2026 23:08

Copilot AI balanced review requested due to automatic review settings October 22, 2026 23:08

Copilot AI reviewed Oct 22, 2026

Gasoonjia reviewed Oct 23, 2026

View reviewed changes

shoumikhin force-pushed the device-pass-constants branch from 894b0fa to ea00fc9 Compare October 23, 2026 02:25

Copilot AI balanced review requested due to automatic review settings October 23, 2026 02:25

Copilot AI reviewed Oct 23, 2026

shoumikhin changed the title ~~Copy constants that feed a device delegate into device memory~~ Fix device placement of constants and shared inputs that feed device delegates Oct 23, 2026

shoumikhin force-pushed the device-pass-constants branch from ea00fc9 to e12db30 Compare October 23, 2026 04:13

Copilot AI balanced review requested due to automatic review settings October 23, 2026 04:13

Copilot AI reviewed Oct 23, 2026

shoumikhin force-pushed the device-pass-constants branch from e12db30 to 71dd63f Compare October 23, 2026 09:44

Copilot AI balanced review requested due to automatic review settings October 23, 2026 09:44

Copilot AI reviewed Oct 23, 2026

shoumikhin changed the title ~~Fix device placement of constants and shared inputs that feed device delegates~~ Fix device placement of constants, shared inputs and delegate outputs Oct 23, 2026

shoumikhin added the ciflow/cuda label Oct 23, 2026

Copilot AI balanced review requested due to automatic review settings October 23, 2026 18:03

shoumikhin force-pushed the device-pass-constants branch from 71dd63f to e1ed5ce Compare October 23, 2026 18:03

shoumikhin requested a review from mergennachin as a code owner October 23, 2026 18:03

Copilot AI reviewed Oct 23, 2026

shoumikhin force-pushed the device-pass-constants branch from e1ed5ce to 635d39e Compare October 23, 2026 19:15

Copilot AI balanced review requested due to automatic review settings October 23, 2026 19:15

shoumikhin added 4 commits October 26, 2026 08:48

shoumikhin force-pushed the device-pass-constants branch from e4244eb to 2a8bff8 Compare October 26, 2026 15:59

Copilot AI balanced review requested due to automatic review settings October 26, 2026 15:59

Copilot AI reviewed Oct 26, 2026

shoumikhin mentioned this pull request Oct 26, 2026

Update the TensorRT backend docs page #24136

Draft

pytorch-bot Bot deployed to upload-benchmark-results October 26, 2026 17:33 Active

Copilot AI balanced review requested due to automatic review settings October 26, 2026 18:55

Copilot AI reviewed Oct 26, 2026

shoumikhin added 2 commits October 26, 2026 12:14

Fix a test comment about how often to_edge converts a constant

d517e67

With _skip_dim_order, to_edge runs convert_constant_dim_order_pass once, not twice, so the comment now covers both modes.

Copilot AI balanced review requested due to automatic review settings October 26, 2026 20:07

Copilot AI reviewed Oct 26, 2026

shoumikhin added 2 commits October 26, 2026 13:34

Copilot AI balanced review requested due to automatic review settings October 26, 2026 20:56

Copilot AI reviewed Oct 26, 2026

Conversation

shoumikhin commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is wrong today

What this change does

What was tested

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

pytorch-bot Bot commented Oct 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/24088

❌ 58 Pending, 5 Unclassified Failures

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Gasoonjia left a comment

Choose a reason for hiding this comment

Uh oh!

Gasoonjia Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

shoumikhin Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Gasoonjia Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

shoumikhin Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

shoumikhin Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

shoumikhin commented Oct 26, 2026

Uh oh!

claude Bot commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: #24088

Summary

Backward Compatibility

Infrastructure

Testing

Recommendation

Uh oh!

shoumikhin commented Oct 26, 2026

Uh oh!

shoumikhin commented Oct 26, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: #24088

Summary

API Design

Code Quality

Testing

Recommendation

Uh oh!

Copilot AI left a comment

shoumikhin commented Oct 22, 2026 •

edited

Loading

pytorch-bot Bot commented Oct 22, 2026 •

edited

Loading

claude Bot commented Oct 26, 2026 •

edited

Loading

claude Bot commented Oct 26, 2026 •

edited

Loading

claude Bot commented Oct 26, 2026 •

edited

Loading

claude Bot commented Oct 26, 2026 •

edited

Loading