Skip to content
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.

Sign up for GitHub

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

Fix: make ggml_backend_qnn_buffer_type_context as static also #1

Closed
wants to merge 3 commits into from
+5 −2
Closed

Fix: make ggml_backend_qnn_buffer_type_context as static also #1

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd927d6
remove new in ggml_backend_qnn_buffer_type
chraac Jun 8, 2024
9c1edfc
Revert "remove new in ggml_backend_qnn_buffer_type"
chraac Jun 8, 2024
355fa5e
another approach
chraac Jun 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions ggml-qnn.cpp
View file
Original file line number Diff line number Diff line change
@@ -3074,12 +3074,15 @@ ggml_backend_buffer_type_t ggml_backend_qnn_buffer_type(size_t device) {
return nullptr;
}

static ggml_backend_qnn_buffer_type_context ggml_backend_qnn_buffer_type_contexts[GGML_QNN_MAX_DEVICES];
static ggml_backend_buffer_type ggml_backend_qnn_buffer_types[GGML_QNN_MAX_DEVICES];

static bool ggml_backend_qnn_buffer_type_initialized = false;

if (!ggml_backend_qnn_buffer_type_initialized) {
for (int i = 0; i < GGML_QNN_MAX_DEVICES; i++) {
for (size_t i = 0; i < GGML_QNN_MAX_DEVICES; i++) {
auto &context = ggml_backend_qnn_buffer_type_contexts[i];
context = { i, std::string(GGML_QNN_NAME) + std::to_string(i) };
ggml_backend_qnn_buffer_types[i] = {
/* .iface = */ {
/* .get_name = */ ggml_backend_qnn_buffer_type_name,
@@ -3090,7 +3093,7 @@ ggml_backend_buffer_type_t ggml_backend_qnn_buffer_type(size_t device) {
/* .supports_backend = */ ggml_backend_qnn_buffer_type_supports_backend,
/* .is_host = */ ggml_backend_qnn_buffer_is_host
},
/* .context = */ new ggml_backend_qnn_buffer_type_context { device, GGML_QNN_NAME + std::to_string(device) },
/* .context = */ &context,
Copy link
Owner

@zhouwg zhouwg Jun 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your PR and thanks for your time. as we discussed in the original PR in upstream GGML comminity, this modification is not make sense(if you are correct, I can't believe there are memory leaks in original ggml backend subsystem and my previous code before your review suggestion here .context is NULL no memory leak issue: you really focus on the language details too much. there are many language masters, but there is only one original author of ggml machine learning framework and they(including the author of ggml backend subsysem) are both modern C++ master). you can find the answer in source code of ggml-backend.c. that's the reason why the original author of ggml backend subystem and Intel's SYCL backend use the same method here.

modification of "int" to "size_t" in for loop is correct.

btw, I really don't think these language details are the "real keypoints" in that PR and I know there are many commercial programmers in China are very very enthusiastic about this although the programming language is really important: language lawyer is awesome but it might be not a good manner in open source community: I'd like to see a programmer build something stuff(for example, the great ggml machine learning framework) but not focus on the language detail again and again.

};
}
ggml_backend_qnn_buffer_type_initialized = true;