Skip to content

Conversation

@foldl
Copy link
Contributor

@foldl foldl commented Feb 18, 2025

Motivation: ggml_is_view_op is a useful API.

Use case 1

It is used by test-backend-ops.cpp.

Use case 2

llama.cpp/src/llama.cpp

Lines 8178 to 8179 in 73e2ed3

if (ggml_backend_supports_op(backend.get(), cur)) {
ggml_backend_sched_set_tensor_backend(lctx.sched.get(), cur, backend.get());

Let's say cur is a view operation and its source is on another backend, then this would be problematic.
if (ggml_backend_supports_op(backend.get(), cur) && !ggml_is_view_op(cur->op)) seems better.

By the way, PR #10825 is useful for checking (visualizing) these scenarios.

@github-actions github-actions bot added testing Everything test related ggml changes relating to the ggml tensor library for machine learning labels Feb 18, 2025
@ggerganov ggerganov requested a review from slaren February 20, 2025 08:37
Copy link
Member

@slaren slaren left a comment

Choose a reason for hiding this comment

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

This function, if made public, does not belong in ggml-backend.cpp. The obvious location would be ggml.c, but moving it there would prevent inlining this function in ggml-backend.cpp, which is not great since it is used very often in ggml_backend_sched.

@foldl
Copy link
Contributor Author

foldl commented Feb 21, 2025

@slaren, I think view_op is a concept defined by backends, so it is ok to be in ggml-backend.cpp. Then, the question is: Is ggml_is_view_op a good name. How about change it to ggml_backend_is_view_op?

@foldl foldl requested a review from slaren February 24, 2025 14:24
@slaren
Copy link
Member

slaren commented Feb 24, 2025

@slaren, I think view_op is a concept defined by backends, so it is ok to be in ggml-backend.cpp. Then, the question is: Is ggml_is_view_op a good name. How about change it to ggml_backend_is_view_op?

No, I don't think this is correct. The operations are defined by the base ggml library, not by the backends. Renaming it to ggml_backend_is_view_op doesn't make sense.

The only way I can see this change would be as a part of some refactor to also move the code of small functions such as this one to a header file so that they can be inlined, but I am not even sure if that would be worth it.

@foldl
Copy link
Contributor Author

foldl commented Feb 24, 2025

The operations are defined by the base library, but how they are handled are defined by the backends.

I am going to close this.

@foldl foldl closed this Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ggml changes relating to the ggml tensor library for machine learning testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants