Skip to content

build: for GGML_BACKEND_DL, ggml need not depend on backend #17709

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

Open
jeffbolznv wants to merge 1 commit into
base: master
Choose a base branch
from
+0 −1
Open

build: for GGML_BACKEND_DL, ggml need not depend on backend #17709

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 0 additions & 1 deletion ggml/src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,6 @@ function(ggml_add_backend_library backend)
# write the shared library to the output directory
set_target_properties(${backend} PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY})
target_compile_definitions(${backend} PRIVATE GGML_BACKEND_DL)
add_dependencies(ggml ${backend})
Copy link
Member

@danbev danbev Dec 3, 2025

Choose a reason for hiding this comment

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

I believe this is a build order dependency and I think it should be kept. add_dependencies(A B) tells CMake that target A depends on target B meaning that whenever we build target A, CMake must first make sure that target B is built.

This makes sure that if we make a change to a backend and then try to build the ggml target, CMake will first rebuild the backend before rebuilding ggml and this keeps things consistent.

If we don't have this it would be possible that after everything is built and we have libggml-base.so, libggml.so, and libggml-vulkan.so in the build directory, and if we then make a change to the ggml-vulkan backend and try building the ggml target, cmake would check ggml and determine that it is up to date and the ggml-vulkan backend would not get rebuilt. So our changes would not be compiled. But with this build order dependency CMake will first rebuild the ggml-vulkan backend before rebuilding ggml.

Copy link
Collaborator Author

@jeffbolznv jeffbolznv Dec 3, 2025

Choose a reason for hiding this comment

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

I think what you're saying is that this dependency is not functionally required, but that people may be using --target ggml as a shorthand to build all ggml libraries, and for some reason not just using --target all. I'm not sure why people would do that, but if you want to preserve that kind of behavior we could add a new target for it:

diff --git a/ggml/src/CMakeLists.txt b/ggml/src/CMakeLists.txt
index 7f2665f75..fe5cf5e17 100644
--- a/ggml/src/CMakeLists.txt
+++ b/ggml/src/CMakeLists.txt
@@ -247,7 +247,11 @@ if (CMAKE_SYSTEM_NAME MATCHES "Linux")
     target_link_libraries(ggml PRIVATE dl)
 endif()

+add_custom_target(ggml_all)
+add_dependencies(ggml_all ggml)
+
 function(ggml_add_backend_library backend)
+    add_dependencies(ggml_all ${backend})
     if (GGML_BACKEND_DL)

Copy link
Member

@danbev danbev Dec 4, 2025

Choose a reason for hiding this comment

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

I think what you're saying is that this dependency is not functionally required

I actually think that this is required to allow the following:

Suppose I use ggml by including it as part of my cmake project. And I've configured GGML_BACKEND_DL=ON to generate dynamic libraries for the backends I want to use that can be loaded at runtime (as opposed to linked at build time).

With the dependency to the backend there, when I build ggml the backends will also be built. Without this dependency only libggml.so and libggml-base.so will be built which might cause issues for project using ggml in this way. Without this those project would have to explicitly build each backend or use --target all.

I'm not sure why people would do that

I think that users/projects that embed ggml might do this in a similar way as described above.

Copy link
Collaborator Author

@jeffbolznv jeffbolznv Dec 4, 2025

Choose a reason for hiding this comment

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

The backends are built because their cmake files get included and those files call ggml_add_backend_library->add_library. It's the add_library call that tells cmake to build it, not the dependency. add_dependencies additionally enforces a build order, and that's what I want to remove.

Copy link
Member

@danbev danbev Dec 4, 2025

Choose a reason for hiding this comment

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

add_dependencies additionally enforces a build order, and that's what I want to remove.

Yes, but I think this is something we might want to keep. My concern is the following scenario.
As an example, we first build with GGML_BACKEND_DL=ON with add_dependencies:

$ cmake --fresh -S . -B build-backends -DCMAKE_BUILD_TYPE=Debug\
    -DGGML_BACKEND_DL=ON \
    -DGGML_CPU_ALL_VARIANTS=ON \
    -DGGML_CUDA=ON \
    -DCMAKE_CUDA_ARCHITECTURES="89"

$ cmake --build build-backends -j8 --target ggml
...
[ 52%] Building CUDA object ggml/src/ggml-cuda/CMakeFiles/ggml-cuda.dir/ggml-cuda.cu.o
[ 54%] Linking CUDA shared module ../../../bin/libggml-cuda.so
[ 97%] Built target ggml-cuda
[100%] Built target ggml

Now if we remove add_dependencies and update the CUDA backend:

$ echo "something" >> ggml/src/ggml-cuda/ggml-cuda.cu

And then build with the same command as above the following will be built:

-- Build files have been written to: /home/danbev/work/ai/llama.cpp/build-backends
[ 50%] Built target ggml-base
[100%] Built target ggml

So the CUDA backend is not built and would not compile if it was. This is the issue I think might have consequences for some users/projects if we remove this.

But if others don't see this as an issue I'm alright with this change. I just mainly wanted to make sure we have considered this scenario.

if (GGML_BACKEND_DIR)
install(TARGETS ${backend} LIBRARY DESTINATION ${GGML_BACKEND_DIR})
else()
Expand Down
Loading