Skip to content

Conversation

cccclai
Copy link
Contributor

@cccclai cccclai commented Apr 8, 2025

Summary: As title, add op_amax to support an internal model, add unit test in test_qnn_delegate.py

Differential Revision: D72613814

Copy link

pytorch-bot bot commented Apr 8, 2025

🔗 Helpful Links

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

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

❌ 1 New Failure

As of commit 553aa3a with merge base a238d6a (image):

NEW FAILURE - The following job has failed:

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

@facebook-github-bot facebook-github-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 Apr 8, 2025
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D72613814

Copy link
Collaborator

@shewu-quic shewu-quic left a comment

Choose a reason for hiding this comment

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

Thanks for your effort.

There is one small thing need to add.

diff --git a/backends/qualcomm/_passes/layout_transform.py b/backends/qualcomm/_passes/layout_transform.py
index 17960a602..4d47c38bc 100644
--- a/backends/qualcomm/_passes/layout_transform.py
+++ b/backends/qualcomm/_passes/layout_transform.py
@@ -47,6 +47,7 @@ class LayoutTransform(ExportPass):
     layout_agnostic_ops = {
         exir_ops.edge.aten.abs.default,
         exir_ops.edge.aten.add.Tensor,
+        exir_ops.edge.aten.amax.default,
         exir_ops.edge.aten.bitwise_or.Tensor,
         exir_ops.edge.aten.bmm.default,
         exir_ops.edge.aten.bitwise_and.Tensor,

cccclai added a commit to cccclai/executorch-1 that referenced this pull request Apr 8, 2025
Summary:

As title, add op_amax to support an internal model, add unit test in test_qnn_delegate.py

Differential Revision: D72613814
@cccclai cccclai force-pushed the export-D72613814 branch from dbeddf3 to b3b857c Compare April 8, 2025 03:55
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D72613814

cccclai added a commit to cccclai/executorch-1 that referenced this pull request Apr 8, 2025
Summary:

As title, add op_amax to support an internal model, add unit test in test_qnn_delegate.py

Differential Revision: D72613814
@cccclai cccclai force-pushed the export-D72613814 branch from b3b857c to bb0b75d Compare April 8, 2025 18:44
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D72613814

@cccclai cccclai added the release notes: qualcomm Changes to the Qualcomm backend delegate label Apr 8, 2025
@cccclai
Copy link
Contributor Author

cccclai commented Apr 8, 2025

Thanks for your effort.

There is one small thing need to add.

diff --git a/backends/qualcomm/_passes/layout_transform.py b/backends/qualcomm/_passes/layout_transform.py
index 17960a602..4d47c38bc 100644
--- a/backends/qualcomm/_passes/layout_transform.py
+++ b/backends/qualcomm/_passes/layout_transform.py
@@ -47,6 +47,7 @@ class LayoutTransform(ExportPass):
     layout_agnostic_ops = {
         exir_ops.edge.aten.abs.default,
         exir_ops.edge.aten.add.Tensor,
+        exir_ops.edge.aten.amax.default,
         exir_ops.edge.aten.bitwise_or.Tensor,
         exir_ops.edge.aten.bmm.default,
         exir_ops.edge.aten.bitwise_and.Tensor,

thanks! updated

Summary:

As title, add op_amax to support an internal model, add unit test in test_qnn_delegate.py

Reviewed By: kirklandsign

Differential Revision: D72613814
@cccclai cccclai force-pushed the export-D72613814 branch from bb0b75d to 553aa3a Compare April 8, 2025 19:32
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D72613814

@cccclai
Copy link
Contributor Author

cccclai commented Apr 8, 2025

Quick question, what is the reason that insert_permute defined in LayoutTransform defaults to False? I currently manually set it to True to continue lowering, but it seems required anyway, correct?

@cccclai cccclai merged commit 46ce593 into pytorch:main Apr 9, 2025
82 of 84 checks passed
@haowhsu-quic
Copy link
Collaborator

Quick question, what is the reason that insert_permute defined in LayoutTransform defaults to False? I currently manually set it to True to continue lowering, but it seems required anyway, correct?

This will actually insert the permute node according to layout information but will break graph if you're currently in partitioner stage (the new layout will not match pytorch cpu convention).
We only turn insert_permute to true in preprocess stage for QNN to build graph correctly.

kirklandsign pushed a commit that referenced this pull request Apr 11, 2025
Summary: As title, add op_amax to support an internal model, add unit
test in test_qnn_delegate.py

Differential Revision: D72613814
keyprocedure pushed a commit to keyprocedure/executorch that referenced this pull request Apr 21, 2025
Summary: As title, add op_amax to support an internal model, add unit
test in test_qnn_delegate.py

Differential Revision: D72613814
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported release notes: qualcomm Changes to the Qualcomm backend delegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants