[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

[EXIR] Register _clone_dim_order op and map aten.clone - #13735

Merged
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot
Sep 4, 2025
Merged

[EXIR] Register _clone_dim_order op and map aten.clone#13735
Gasoonjia merged 27 commits into
pytorch:mainfrom
keyprocedure:add-dim-order-clone-aot

Conversation

@keyprocedure

@keyprocedurekeyprocedure commented Aug 27, 2025

Copy link
Copy Markdown
Contributor

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

pytorch-botBot commented Aug 27, 2025

Copy link
Copy Markdown

🔗 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-clameta-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 Aug 27, 2025
@keyprocedure

Copy link
Copy Markdown
ContributorAuthor

@pytorchbot label "release notes: none"

@pytorch-botpytorch-botBot added the release notes: none Do not include this in the release notes label Aug 27, 2025
@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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

@keyprocedure

keyprocedure commented Sep 3, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

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

@keyprocedure

keyprocedure commented Sep 4, 2025

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown

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
Copy Markdown

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.

@Gasoonjia

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@GasoonjiaGasoonjia 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.

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:mainSep 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 <gasoonjia@meta.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ciflow/trunkCLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.release notes: noneDo 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

5 participants

@keyprocedure@Gasoonjia@metascroy@nil-is-all@digantdesai