Allow qnn to use the IR from torch.export.export#4942
Allow qnn to use the IR from torch.export.export#4942facebook-github-bot merged 18 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/4942
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 8ec839f with merge base f55ce1f ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
cccclai
changed the title
|
Hi @shewu-quic @chiwwang, could you let me know if this change meets the need? Thanks. |
|
Thank you! This looks ok. We will give it a try. |
shewu-quic
left a comment
shewu-quic
left a comment
There was a problem hiding this comment.
Thanks for your effort.
It works well for me.
|
Great. Just apply suggestion |
|
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| if self.export_fn == torch.export.export: | ||
| self.pre_autograd_graph_module = self.export_fn( | ||
| self.model, | ||
| self.example_inputs, | ||
| dynamic_shapes=dynamic_shape, | ||
| strict=True, | ||
| ).module() | ||
| else: | ||
| self.pre_autograd_graph_module = self.export_fn( | ||
| self.model, self.example_inputs, dynamic_shapes=dynamic_shape | ||
| ) |
There was a problem hiding this comment.
Why do you need this if you already patch export_fn
There was a problem hiding this comment.
And if this is because of differing APIs then I would rather check for qnn backend here instead of using export_fn
|
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| self.pre_autograd_graph_module = capture_pre_autograd_graph( | ||
| self.model, self.example_inputs, dynamic_shapes=dynamic_shape | ||
| ) | ||
| if self.args.qnn: |
There was a problem hiding this comment.
nit: do we need to pass entire set of args? Can we not just pass in backend information? or is_qnn?
There was a problem hiding this comment.
So I'm not sure it's qnn specific and I feel like maybe coreml/mps might run into similar issue if they use pt2e flow....
There was a problem hiding this comment.
The current backend information is args.qnn, args.coreml and args.mps...We can refactor it to make it cleaner.
There was a problem hiding this comment.
So I'm not sure it's qnn specific and I feel like maybe coreml/mps might run into similar issue if they use pt2e flow....
Yes... agreed on this 😮
Is there any other backend trying the pt2e flow? (i.e., going through convert_pt2e())
kimishpatel
left a comment
kimishpatel
left a comment
There was a problem hiding this comment.
So i dont really like the kind of spaghetti this code has become, but that no the fault of the author and we need to unblock qnn delegate. So stamping
|
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
5 participants
It is a workaround for #4627