Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add QNN EP HTP shared memory allocator #23136
base: main
Are you sure you want to change the base?
Add QNN EP HTP shared memory allocator #23136
Changes from 43 commits
110a3bc
0ba3a2f
8436b14
c9826f4
c07c35e
60dc837
24e072f
e66cbef
8c515da
18e2780
09ddce5
a65bb71
bfb135e
f179a0d
7645ef4
1043732
022f4bc
c527dee
e4f72b3
39ff901
1bed5a4
d70db84
45ef883
d2e7b3c
c892c18
b295eef
13f5e30
d5eace1
bacbcdc
30cd9ed
b29ab61
67a54b8
c0569e2
dd45c84
959d8df
ab48516
4a3f6c3
65ce4b1
ff12541
5e6e103
78e86cc
e665a2b
425023b
781a4a0
4d29208
568c9a7
8b95535
515999c
6986839
00b286b
88dec64
4ca3ea7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 1678 in onnxruntime/core/providers/qnn/builder/qnn_backend_manager.cc
GitHub Actions / Optional Lint C++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be true as long as QNN contexts are not freed from anywhere other than the destructor.
it seems kind of brittle to depend on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't we catch it during development if someone changed the code to free the context somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the concern is a race between this clean up function locking weak_context_mem_handle_manager (thus keeping it alive) and the QNN context handle getting freed.
I'm thinking it may be possible to manage the QNN context handle (as well as the context mem handles) in some object and have a weak_ptr to that instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log something if either of these are false or is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the weak_ptrs wouldn't be able to be locked in the case where the backend manager (i.e., the QNN EP) is destroyed before the allocation is freed. currently it should be fine for the allocator to outlive the EP. so it seems like this case is not too unexpected.
Check warning on line 1726 in onnxruntime/core/providers/qnn/builder/qnn_backend_manager.cc
GitHub Actions / Optional Lint C++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be private if it's not meant to be called directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally it would be private, but then std::make_shared wouldn't be able to access it