Skip to content

Conversation

@keyprocedure
Copy link
Contributor

@keyprocedure keyprocedure commented Aug 27, 2025

Summary

This is PR 2 of 3 implementing a dim order aware clone op.

This PR registers the new _clone_dim_order op and maps aten.clone to dim_order_ops._clone_dim_order in EXIR during export to preserve memory layout changes (contiguous/channels_last). It also updates the Core ML, ARM, and Qualcomm backends to handle the new clone op.

Related PRs:

  • PR 1: #12974 - Add _clone_dim_order portable kernel
  • PR 3: #12976 - Update RemoveCloneOpsTransform to be dim order aware

Fixes #12645

Test plan

  • Operator level tests to verify kernel behavior for layout preservation and changes.
  • Graph level checks to confirm that clone mapping occurs.
  • End to end tests to validate that functional clone behavior is unchanged.
  • Backend tests to ensure clone semantics are preserved.

All tests pass via:
python -m unittest exir.tests.test_memory_format_ops_pass
python -m unittest backends.apple.coreml.test.test_torch_ops
pytest backends/arm/test/ops/test_clone.py
pytest backends/arm/test/passes/test_remove_clone_pass.py

@pytorch-bot
Copy link

pytorch-bot bot commented Aug 27, 2025

🔗 Helpful Links

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

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

❌ 3 New Failures

As of commit 30a1d13 with merge base 32e82bc (image):

NEW FAILURES - The following jobs have failed:

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 Aug 27, 2025
@keyprocedure
Copy link
Contributor Author

@pytorchbot label "release notes: none"

@pytorch-bot pytorch-bot bot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@pytorch-bot pytorch-bot bot removed the ciflow/trunk label Sep 3, 2025
@keyprocedure
Copy link
Contributor Author

keyprocedure commented Sep 3, 2025

@Gasoonjia I could use your input on the Qualcomm backend changes:

After replacing edge.aten.clone with edge.dim_order_ops._clone_dim_order in QNN, I was able to successfully run all the previously failing QNN tests on a local Linux machine.

Should _clone_dim_order always be treated as a non-delegated op, like clone was?

I set the clone op removal condition to the default value (True) instead of checking if dim order is non-contiguous.

I also didn't create a node visitor or add an operator test in test_qnn_delegate since the op wouldn't be delegated and would fail the delegated partitioner count test.

@Gasoonjia
Copy link
Contributor

Hi @keyprocedure , thanks for your great work and ci has been triggered!

Yes we can treat the dim order variant copy as non-delegated, just as what we are doing for aten::copy.

cc. @digantdesai

@keyprocedure
Copy link
Contributor Author

Happy to help!
Sounds good, really appreciate your responsiveness.

@keyprocedure
Copy link
Contributor Author

keyprocedure commented Sep 3, 2025

@Gasoonjia CI failures are unrelated. If everything looks good to you, I can commit the test_torch_ops.py merge conflict fix.

@Gasoonjia
Copy link
Contributor

@keyprocedure lgtm. Please solve the conflict and then I will stamp it!

@pytorch-bot pytorch-bot bot removed the ciflow/trunk label Sep 4, 2025
@keyprocedure
Copy link
Contributor Author

keyprocedure commented Sep 4, 2025

@Gasoonjia the merge conflict is resolved.

I noticed that coreml's torch_ops.py has updated the way it registers _empty_dim_order and _to_dim_order_copy. Since _clone_dim_order hasn't been registered in Apple's dim_order_ops.py yet, are the current changes in torch_ops.pysufficient for now?

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 4, 2025

To add the ciflow label ciflow/trunk please first approve the workflows that are awaiting approval (scroll to the bottom of this page).

This helps ensure we don't trigger CI on this PR until it is actually authorized to do so. Please ping one of the reviewers if you do not have access to approve and run workflows.

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 4, 2025

To add the ciflow label ciflow/trunk please first approve the workflows that are awaiting approval (scroll to the bottom of this page).

This helps ensure we don't trigger CI on this PR until it is actually authorized to do so. Please ping one of the reviewers if you do not have access to approve and run workflows.

@pytorch-bot pytorch-bot bot removed the ciflow/trunk label Sep 4, 2025
@Gasoonjia
Copy link
Contributor

@keyprocedure i think it is fine for us right now, and we can work on coreml side later.

@metascroy do you have any concerns on this?

to(context, node)


@register_torch_op(
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@metascroy
Copy link
Contributor

@Gasoonjia the merge conflict is resolved.

I noticed that coreml's torch_ops.py has updated the way it registers _empty_dim_order and _to_dim_order_copy. Since _clone_dim_order hasn't been registered in Apple's dim_order_ops.py yet, are the current changes in torch_ops.pysufficient for now?

Yes, the changes in torch_ops.py should be sufficient. The purpose of torch_ops.py is to register ops that are not yet in coremltools. With that said, we generally prefer that you put up a PR in coremltools and link that PR in a comment above the op you're creating in toch_ops.py. You can see this for most other ops in the file, e.g.,

# https://github.com/apple/coremltools/pull/2557

Copy link
Contributor

@Gasoonjia Gasoonjia left a comment

Choose a reason for hiding this comment

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

Thanks @metascroy 's comment! Let me stamp this PR for now and we can add coreml PR later.

@Gasoonjia Gasoonjia merged commit c5ff74c into pytorch:main Sep 4, 2025
299 of 303 checks passed
Gasoonjia added a commit that referenced this pull request Sep 9, 2025
### Summary
This is PR 3 of 3 implementing a dim order aware clone op. 

This PR updates the clone removal pass to retain layout changing
`aten.clone` and `_clone_dim_order` ops and remove no-op clones,
ensuring layout/dim order is preserved through export.

Related PRs:
- PR 1: [#12974](#12974) - Add
`_clone_dim_order` portable kernel
- PR 2: [#13735](#13735) -
Register `_clone_dim_order` op and map `aten.clone`

Fixes #12645 

### Test plan
Added tests to verify:
- Clones that change layout are preserved.
- Clones with unchanged layout (identity ops) are removed.

All tests pass via:
python -m unittest exir.tests.test_memory_format_ops_pass
python -m unittest backends.transforms.test.test_remove_clone_ops

---------

Co-authored-by: Gasoonjia <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunk CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: none Do not include this in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add dim order variant clone operator

4 participants