Qualcomm AI Engine Direct - multi-method support by haowhsu-quic · Pull Request #10584 · pytorch/executorch

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic · 2025-04-30T15:23:59Z

Summary

refactor to adopt multi-method change
framework patch to meet multi-method use case

Test plan

python backends/qualcomm/tests/test_qnn_delegate.py -k TestQNNQuantizedUtils.test_qnn_backend_multi_graphs -s $device_sn -b build-android -m SM8750

pytorch-bot · 2025-04-30T15:24:02Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR

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

✅ No Failures

As of commit 0abe0cc with merge base ed718a8 ():
💚 Looks good so far! There are no failures yet. 💚

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

haowhsu-quic · 2025-04-30T15:24:19Z

@pytorchbot label "release notes: qualcomm"

cccclai · 2025-05-01T09:01:39Z

 owning_graph_module = call_submodule_node.graph.owning_module
 # call delegate args should only use user_inputs
 call_delegate_args = []
+ # handle getitem node in multi-method scenario


Can you share what issues you run into?

The scenario happens when there are common nodes shared by multiple delegated subgraphs (e.g. frequency sin/cos in sharded llama):
The example graph looks like below:

When replacing submodule fused_qnn_1, the original name finding mechanism are trying to match between:
submodule_program.graph_signature.user_inputs: ('aten_mean_dim', 'aten_select_int', 'aten_select_int_1')
call_submodule_node.all_input_nodes: [aten_mean_dim, getitem_1, getitem_2]
Which makes getitem node dangling as following, since the names could not be correctly mapped:

The patch here is trying to find the original graph with real output names and use index of getitem to have them in correct order.
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program.
This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate.
But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

@mcr229 @angelayi can you help double check the logic here?

@haowhsu-quic thanks for finding this edge case here. Let me add a test for this internally, so that it is captured by CI. I'm surprised that the exported_program's call signature doesn't have the updated get_items. This suggests to me that they aren't be created in topological order. I'll take a look and see if there is a fix that needs to be applied higher up to enforce this.
Thanks for the debugging and finding the root cause!

Thank you! There is another issue mentioned in last section, could you help check it as well?
And I think another issue is about validation in _unsafe_adjust_original_program. Since the partitioned sub graphs in multi-method scenario have already been turned into submodules (like first diagram). That behavior will make original_program._validate() fail in _unsafe_adjust_original_program. This does not happen in single method lowering because sub graphs are turned into submodules one by one and replaced into executorch_call_delegate. But I cannot find an appropriate way to pass official CI, could you give me some hint? thank you!

Thank you for helping resolve the issues.

Which makes getitem node dangling as following, since the names could not be correctly mapped:
I'm curious if DCE can prune the dangling getitem nodes. Have you tried to run the DCE pass right after the mapping is done?

I think the graph with dangling getitem nodes is not the correct behavior, so it's not about removing them.

facebook-github-bot · 2025-05-20T16:27:38Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cccclai · 2025-05-20T17:22:42Z

It needs to rebase..

Summary - refactor to adopt multi-method change - framwork change to meet use case

facebook-github-bot · 2025-05-22T00:34:44Z

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046

haowhsu-quic requested review from JacobSzwejbka, cccclai, larryliu0820 and tarun292 as code owners April 30, 2025 15:24

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 30, 2025

pytorch-botBot added the release notes: qualcomm Changes to the Qualcomm backend delegate label Apr 30, 2025

haowhsu-quic force-pushed the dev_multi_method branch 2 times, most recently from 0dd1e57 to 8468fa7Compare May 1, 2025 04:49

cccclai reviewed May 1, 2025
View reviewed changes

haowhsu-quic force-pushed the dev_multi_method branch from 30883fd to c2e6f5aCompare May 15, 2025 03:49

haowhsu-quic requested review from jathu and kirklandsign as code owners May 15, 2025 03:49

Qualcomm AI Engine Direct - multi-method support
6761ffb
Summary - refactor to adopt multi-method change - framwork change to meet use case

haowhsu-quic force-pushed the dev_multi_method branch from 8fc3fc5 to 6761ffbCompare May 22, 2025 00:13

Merge branch 'main' into dev_multi_method
0abe0cc

cccclai approved these changes May 22, 2025
View reviewed changes

kirklandsign approved these changes May 22, 2025
View reviewed changes

facebook-github-bot merged commit f6ba65b into pytorch:mainMay 22, 2025

cccclai mentioned this pull request May 22, 2025
forward fix #11071
Merged

cccclai added a commit to cccclai/executorch-1 that referenced this pull request May 22, 2025
forward fix
cf1a75b
Summary: Forward fix for pytorch#10584 Reviewed By: kirklandsign Differential Revision: D75231046

cccclai added a commit to cccclai/executorch-1 that referenced this pull request May 22, 2025
forward fix (pytorch#11071)
a68decb
Summary: Forward fix for pytorch#10584 Reviewed By: digantdesai, kirklandsign Differential Revision: D75231046