Skip to content

ggml: link MATH_LIBRARY not by its full path #9339

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

Jump to bottom
Merged
merged 1 commit into from
Sep 16, 2024
Merged

ggml: link MATH_LIBRARY not by its full path #9339

merged 1 commit into from
Sep 16, 2024

Conversation

Xarbirus
Copy link
Contributor

@Xarbirus Xarbirus commented Sep 6, 2024

The current MATH_LIBRARY linkage method works if the llama.cpp library is installed on the same machine where it will be used. If the built and installed artefacts were transferred to another machine, an error like this may occur:

error: '/Applications/Xcode_14.3.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.3.sdk/usr/lib/libm.tbd', needed by '***/llama-inference', missing and no known rule to make it

due to the fact that the llama-config.cmake specifies the full path to the library. The proposed fix changes the config to
set(_llama_link_deps "${ggml_LIBRARY}" "Threads::Threads;m"). Which helps with portability.

@ggerganov
Copy link
Member

ggerganov commented Sep 7, 2024 •
edited
Loading

The proposed solution generates the following warning on my system:

[ 63%] Linking CXX shared library libggml.dylib
cd /Users/ggerganov/development/github/llama.cpp/build/ggml/src && /opt/homebrew/Cellar/cmake/3.27.1/bin/cmake -E cmake_link_script CMakeFiles/ggml.dir/link.txt --verbose=1
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -O3 -DNDEBUG -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk -dynamiclib -Wl,-headerpad_max_install_names -o libggml.dylib -install_name @rpath/libggml.dylib CMakeFiles/ggml.dir/ggml.c.o "CMakeFiles/ggml.dir/ggml-alloc.c.o" "CMakeFiles/ggml.dir/ggml-backend.c.o" "CMakeFiles/ggml.dir/ggml-quants.c.o" "CMakeFiles/ggml.dir/ggml-metal.m.o" "CMakeFiles/ggml.dir/__/__/autogenerated/ggml-metal-embed.s.o" "CMakeFiles/ggml.dir/ggml-blas.cpp.o" CMakeFiles/ggml.dir/llamafile/sgemm.cpp.o "CMakeFiles/ggml.dir/ggml-aarch64.c.o"  -Xlinker -framework -Xlinker Accelerate -Xlinker -framework -Xlinker Foundation -Xlinker -framework -Xlinker Metal -Xlinker -framework -Xlinker MetalKit -Xlinker -framework -Xlinker Accelerate -lm -Xlinker -framework -Xlinker Foundation -Xlinker -framework -Xlinker Metal -Xlinker -framework -Xlinker MetalKit -lm 
ld: warning: ignoring duplicate libraries: '-lm'

Isn't it better to do something like this instead:

diff --git a/ggml/src/CMakeLists.txt b/ggml/src/CMakeLists.txt
index cd2dcd06..0c7b5cad 100644
--- a/ggml/src/CMakeLists.txt
+++ b/ggml/src/CMakeLists.txt
@@ -1334,6 +1334,9 @@ find_library(MATH_LIBRARY m)
 if (MATH_LIBRARY)
     if (NOT WIN32 OR NOT GGML_SYCL)
         target_link_libraries(ggml PRIVATE ${MATH_LIBRARY})
+        if (BUILD_SHARED_LIBS)
+            set_target_properties(ggml PROPERTIES INTERFACE_LINK_LIBRARIES "m")
+        endif()
     endif()
 endif()

@Xarbirus Xarbirus mentioned this pull request Sep 12, 2024
Merged
4 tasks
@Xarbirus
Copy link
Contributor Author

I think I found one problem. Even now the default build from the master branch on MacOS results in a duplicate problem.
When GGML_ACCELERATE and GGML_BLAS are ON and GGML_BLAS_VENDOR_DEFAULT is Apple, we link with Accelerate framework twice.
build/ggml/src/CMakeFiles/ggml.dir/link.txt:

/Library/Developer/CommandLineTools/usr/bin/c++ -g -arch arm64 \
-isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk \
-dynamiclib -Wl,-headerpad_max_install_names -o libggml.dylib -install_name @rpath/libggml.dylib \
CMakeFiles/ggml.dir/ggml.c.o "CMakeFiles/ggml.dir/ggml-alloc.c.o" \
"CMakeFiles/ggml.dir/ggml-backend.c.o" "CMakeFiles/ggml.dir/ggml-quants.c.o" \
"CMakeFiles/ggml.dir/ggml-blas.cpp.o" CMakeFiles/ggml.dir/llamafile/sgemm.cpp.o \
"CMakeFiles/ggml.dir/ggml-aarch64.c.o" \
-framework Accelerate \
-framework Accelerate \
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/lib/libm.tbd

But when Metal (GGML_METAL is ON) comes into play, we get a more complex picture.
build/ggml/src/CMakeFiles/ggml.dir/link.txt:

/Library/Developer/CommandLineTools/usr/bin/c++ -g -arch arm64 \
-isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk \
-dynamiclib -Wl,-headerpad_max_install_names -o libggml.dylib -install_name @rpath/libggml.dylib \
CMakeFiles/ggml.dir/ggml.c.o "CMakeFiles/ggml.dir/ggml-alloc.c.o" \
"CMakeFiles/ggml.dir/ggml-backend.c.o" "CMakeFiles/ggml.dir/ggml-quants.c.o" \
"CMakeFiles/ggml.dir/ggml-metal.m.o" "CMakeFiles/ggml.dir/__/__/autogenerated/ggml-metal-embed.s.o" \
"CMakeFiles/ggml.dir/ggml-blas.cpp.o" CMakeFiles/ggml.dir/llamafile/sgemm.cpp.o \
"CMakeFiles/ggml.dir/ggml-aarch64.c.o" \
-framework Accelerate \
-framework Foundation \
-framework Metal \
-framework MetalKit \
-framework Accelerate \
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/lib/libm.tbd \
-framework Foundation \
-framework Metal \
-framework MetalKit \
/Library/Developer/CommandLineTools/SDKs/MacOSX14.4.sdk/usr/lib/libm.tbd

Thus, all libraries and frameworks end up being duplicated. Moreover, this problem appears only with all three variables (GGML_ACCELERATE, GGML_BLAS and GGML_METAL) ON, any other two do not duplicate libm. It seems that the solution to the problem is to eliminate Accelerate double linking. But I'm not a professional in CMake, maybe you know another option or knew about this problem? I'll try to post my solution today.

@ggerganov ggerganov mentioned this pull request Sep 13, 2024
Merged
4 tasks
@ggerganov
Copy link
Member

I'm also not 100% confident about the correct CMake way, though here is an attempt to remove fix the double linkage: #9463. PTAL

@Xarbirus
Copy link
Contributor Author

@ggerganov yes, that was exactly my solution! Rebased my branch onto gg/cmake-dedup-link

@Xarbirus
Copy link
Contributor Author

Waiting for #9463 to be merged.

@Xarbirus
Copy link
Contributor Author

Also added a change to avoid duplicating the m library with SYCL. And placed public libraries in a separate list.

@ggerganov
Copy link
Member

Let's rebase on latest master

@Xarbirus
Copy link
Contributor Author

Finally the work is finished😊

@ggerganov ggerganov merged commit a6a3a5c into ggml-org:master Sep 16, 2024
51 checks passed
@Xarbirus Xarbirus deleted the math_library_link branch September 16, 2024 20:37
dsx1986 pushed a commit to dsx1986/llama.cpp that referenced this pull request Oct 29, 2024
@Xarbirus @dsx1986
ggml : link MATH_LIBRARY not by its full path (ggml-org#9339)
97fc86a
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 15, 2024
@Xarbirus @arthw
ggml : link MATH_LIBRARY not by its full path (ggml-org#9339)
dd567b4
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Nov 18, 2024
@Xarbirus @arthw
ggml : link MATH_LIBRARY not by its full path (ggml-org#9339)
5fe91d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggerganov ggerganov ggerganov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Xarbirus @ggerganov