Support optrace #6535
Support optrace #6535
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/6535
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D64941627 |
cccclai
requested review from
chiwwang,
chunit-quic,
haowhsu-quic and
shewu-quic
|
@chiwwang Limin added support for for op trace? Mind taking a look? |
| if (profile_level != QnnExecuTorchProfileLevel::kProfileOff) { | ||
| const QnnInterface& qnn_interface = implementation_.GetQnnInterface(); | ||
| Qnn_ErrorHandle_t error = qnn_interface.qnn_profile_create( | ||
| backend_->GetHandle(), static_cast<int>(profile_level), &handle_); |
There was a problem hiding this comment.
Do you want to fixed profile_level?
There was a problem hiding this comment.
maybe we should add the level to QnnExecuTorchProfileLevel. Maybe typo? I saw kProfileOptrace = 3 there.
@shewu-quic , how about the linting profile level? Should we reserve a number for that?
There was a problem hiding this comment.
Not typo. Optrace requires profiling level to be QNN_PROFILE_LEVEL_DETAILED (2), same as kProfileDetailed. I considered adding an additional config for optrace, but it requires signature change of many functions, so I chose to add a new profiling level kProfileOptrace in the schema, but the profiling level passed to QNN compiler for which is the same as kProfileDetailed.
There was a problem hiding this comment.
Updated to remove the hardcoded value of QNN profile level.
chiwwang
left a comment
chiwwang
left a comment
There was a problem hiding this comment.
Thank your for the change!
Generally looks good for me. Just some comments around the profile_level settings.
| if (profile_level != QnnExecuTorchProfileLevel::kProfileOff) { | ||
| const QnnInterface& qnn_interface = implementation_.GetQnnInterface(); | ||
| Qnn_ErrorHandle_t error = qnn_interface.qnn_profile_create( | ||
| backend_->GetHandle(), static_cast<int>(profile_level), &handle_); |
There was a problem hiding this comment.
maybe we should add the level to QnnExecuTorchProfileLevel. Maybe typo? I saw kProfileOptrace = 3 there.
@shewu-quic , how about the linting profile level? Should we reserve a number for that?
| online_prepare: bool = False, | ||
| dump_intermediate_outputs: bool = False, | ||
| profile: bool = False, | ||
| optrace: bool = False, |
There was a problem hiding this comment.
How about change profile: bool to an enum class or something similar? Then users can choose profile level they want.
There was a problem hiding this comment.
Good point. However, before this change kProfileDetailed is the only supported profile level. It might be better to make this change in a separate diff.
Summary: Add a new profile level to support optrace capture. Reviewed By: billmguo Differential Revision: D64941627
limintang
force-pushed
the
export-D64941627
branch
from
9e9f033 to
0c292cf
Compare
|
This pull request was exported from Phabricator. Differential Revision: D64941627 |
Summary: Add a new profile level to support optrace capture. Reviewed By: billmguo Differential Revision: D64941627
limintang
force-pushed
the
export-D64941627
branch
from
0c292cf to
1908721
Compare
|
This pull request was exported from Phabricator. Differential Revision: D64941627 |
shewu-quic
left a comment
shewu-quic
left a comment
There was a problem hiding this comment.
LGTM. Thanks for your effort.
|
Looks good for me. Thank you! |
cccclai
left a comment
cccclai
left a comment
There was a problem hiding this comment.
Thanks folk for the review!
limintang
left a comment
limintang
left a comment
There was a problem hiding this comment.
@pytorchbot merge
|
Mergebot is not configured for this repository. Please use the merge button provided by GitHub. |
5 participants
Differential Revision: D64941627