llama.cpp icon indicating copy to clipboard operation
llama.cpp copied to clipboard

cmake: simplify vulkan shader test logic

Open bandoti opened this issue 7 months ago • 6 comments

This PR adds a CMake function to simplify the shader feature-detection logic. In addition, the requisite preprocessor defines are shared with the vulkan-shaders-gen project through an auto-generated cmake file, preventing the need to pass them along manually.

bandoti avatar May 02 '25 12:05 bandoti

@jeffbolznv I think you were right about using the external project add for vulkan-shaders-gen for both the cross-compile and the regular build. I am going to try that here now I guess the timing is right. 😊

bandoti avatar May 02 '25 13:05 bandoti

Thanks for making these cleanups. The changes look reasonable to me, though I don't know if there are any weird consequences of passing cmake arguments via file.

jeffbolznv avatar May 02 '25 13:05 jeffbolznv

No problem. We'll see how the CI responds. I added a couple cross-compile builds there so at very least we have the basic compile-checks covered.

bandoti avatar May 02 '25 13:05 bandoti

Interesting. This doesn't seem much simpler, though. It's about as much code and going through an external file is slightly convoluted. But I don't know enough about cmake to judge that particularly well, so I'll leave it up to you to decide what is better.

0cc4m avatar May 04 '25 04:05 0cc4m

The primary motivation is to only change one place when new extension checks are added.

Because the Vulkan-shaders-gen project had several bugs associated with missing defines, this change would ensure that they will be present without having to duplicate flags within the sub project.

Also, the "subdirs" loading is replaced by unifying the cross-compile approach with a regular build, both using ExternalProject_Add now.

bandoti avatar May 04 '25 11:05 bandoti

The one issue with using the file copy, however, is that one cannot build the Vulkan-shaders-gen without building ggml-vulkan, and so it becomes an implementation detail despite being an external project.

I am happy to undo that change if this is not desirable. From my point of view this is the only way I've built the app, but perhaps there's a reason to decouple them.

The net effect is just that we need to update 3 spots instead of 1 when adding new extension capabilities.

bandoti avatar May 06 '25 15:05 bandoti

I went ahead and removed the auto-generated file, but as a compromise the set of extension arguments are appended to a list which gets expanded automatically when adding vulkan-shaders-gen project.

So, when adding new extension checks we just need to make two changes:

  1. In ggml/src/ggml-vulkan/CMakeLists.txt:
    test_shader_extension_support(
        "GL_KHR_cooperative_matrix"
        "${CMAKE_CURRENT_SOURCE_DIR}/vulkan-shaders/test_coopmat_support.comp"
        "GGML_VULKAN_COOPMAT_GLSLC_SUPPORT"
    )
  1. And ggml/src/ggml-vulkan/vulkan-shaders/CMakeLists.txt:
if (GGML_VULKAN_COOPMAT_GLSLC_SUPPORT)
    add_compile_definitions(GGML_VULKAN_COOPMAT_GLSLC_SUPPORT)
    message(STATUS "Enabling coopmat glslc support")
endif()

Well, and of course the shader test itself... 😅

bandoti avatar May 13 '25 12:05 bandoti

@0cc4m glad I could be of help. Learning Vulkan is on my bucket list but I'm not there yet—maybe some day I'll be able to help you all with the actual work! 😊

bandoti avatar May 15 '25 18:05 bandoti

I'm now getting the following build failure. With this patch (09d13d94fb169093095416ac6323551c37b75339). If I use the previous commit (24e86cae7219b0f3ede1d5abdf5bf3ad515cccb8) llama.cpp builds without error. I'm building using GGML_VULKAN.

Any tips on how to now get llama.cpp to build?

llama-cpp> [37/247] Performing build step for 'vulkan-shaders-gen'
llama-cpp> [1/2] Building CXX object CMakeFiles/vulkan-shaders-gen.dir/vulkan-shaders-gen.cpp.o
llama-cpp> [2/2] Linking CXX executable /build/source/build/bin/vulkan-shaders-gen
llama-cpp> [38/247] Performing install step for 'vulkan-shaders-gen'
llama-cpp> FAILED: ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-stamp/vulkan-shaders-gen-install /build/source/build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-stamp/vulkan-shaders-gen-install
llama-cpp> cd /build/source/build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-build && /nix/store/wphngc22a7aphbp5pi5jqmqlsqmisgn5-cmake-3.31.6/bin/cmake --install . && /nix/store/wphngc22a7aphbp5pi5jqmqlsqmisgn5-cmake-3.31.6/bin/cmake -E touch /build/source/build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-stamp/vulkan-shaders-gen-install
llama-cpp> -- Install configuration: ""
llama-cpp> CMake Error at cmake_install.cmake:52 (file):
llama-cpp>   file INSTALL cannot find "/build/source/build/bin/vulkan-shaders-gen": No
llama-cpp>   such file or directory.
llama-cpp> ninja: build stopped: subcommand failed.

quag avatar May 25 '25 23:05 quag

@quag sorry this is causing an issue! What is your build environment?

Looks like the path "ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix" is bad with the "-prefix" part.

It could be related to #13753 I'm thinking. Will take a look as soon as I have time, but in the meantime knowing what system setup you're building with may help! Thanks.

bandoti avatar May 26 '25 16:05 bandoti

@bandoti thanks! I'm building on NixOS with a x86_64 system on an AMD GPU. (I find that vulkan works better than ROCm and is generally less hassle.) Is that what you meant by "What's your build environment?"

https://github.com/ggml-org/llama.cpp/issues/13753#issuecomment-2909433032 looks related. I'll give the proposed change to ggml/src/ggml-vulkan/CMakeLists.txt a go and will report back if it works for me or not.


Update: No, the proposed CMakeLists.txt in https://github.com/ggml-org/llama.cpp/issues/13753#issuecomment-2909433032 does not build for me. I get the following error.

llama-cpp> [8/233] Performing configure step for 'vulkan-shaders-gen'
llama-cpp> FAILED: ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-stamp/vulkan-shaders-gen-configure /build/source/build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-stamp/vulkan-shaders-gen-configure
llama-cpp> cd /build/source/build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-build && /nix/store/wphngc22a7aphbp5pi5jqmqlsqmisgn5-cmake-3.31.6/bin/cmake -DCMAKE_INSTALL_PREFIX=/build/source/build -DCMAKE_RUNTIME_OUTPUT_DIRECTORY=/build/source/build/bin -DGGML_VULKAN_COOPMAT_GLSLC_SUPPORT=ON -GNinja -S /build/source/ggml/src/ggml-vulkan/vulkan-shaders -B /build/source/build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-build && /nix/store/wphngc22a7aphbp5pi5jqmqlsqmisgn5-cmake-3.31.6/bin/cmake -E touch /build/source/build/ggml/src/ggml-vulkan/vulkan-shaders-gen-prefix/src/vulkan-shaders-gen-stamp/vulkan-shaders-gen-configure
llama-cpp> CMake Warning (dev) in CMakeLists.txt:
llama-cpp>   No project() command is present.  The top-level CMakeLists.txt file must
llama-cpp>   contain a literal, direct call to the project() command.  Add a line of
llama-cpp>   code such as
llama-cpp>
llama-cpp>     project(ProjectName)
llama-cpp>
llama-cpp>   near the top of the file, but after cmake_minimum_required().
llama-cpp>
llama-cpp>   CMake is pretending there is a "project(Project)" command on the first
llama-cpp>   line.
llama-cpp> This warning is for project developers.  Use -Wno-dev to suppress it.
llama-cpp>
llama-cpp> CMake Warning (dev) in CMakeLists.txt:
llama-cpp>   cmake_minimum_required() should be called prior to this top-level project()
llama-cpp>   call.  Please see the cmake-commands(7) manual for usage documentation of
llama-cpp>   both commands.
llama-cpp> This warning is for project developers.  Use -Wno-dev to suppress it.
llama-cpp>
llama-cpp> -- The C compiler identification is GNU 14.2.1
llama-cpp> -- The CXX compiler identification is GNU 14.2.1
llama-cpp> -- Detecting C compiler ABI info
llama-cpp> -- Detecting C compiler ABI info - done
llama-cpp> -- Check for working C compiler: /nix/store/0fsnicvfpf55nkza12cjnad0w84d6ba7-gcc-wrapper-14.2.1.20250322/bin/gcc - skipped
llama-cpp> -- Detecting C compile features
llama-cpp> -- Detecting C compile features - done
llama-cpp> -- Detecting CXX compiler ABI info
llama-cpp> -- Detecting CXX compiler ABI info - done
llama-cpp> -- Check for working CXX compiler: /nix/store/0fsnicvfpf55nkza12cjnad0w84d6ba7-gcc-wrapper-14.2.1.20250322/bin/g++ - skipped
llama-cpp> -- Detecting CXX compile features
llama-cpp> -- Detecting CXX compile features - done
llama-cpp> -- Found Vulkan: /nix/store/nszik1q8ffmvsqk54kbc75dwyxwvi2nm-vulkan-loader-1.4.313.0/lib/libvulkan.so (found version "1.4.313") found components: glslc missing components: glslangValidator
llama-cpp> CMake Error at CMakeLists.txt:5 (add_subdirectory):
llama-cpp>   add_subdirectory given source "vulkan-shaders" which is not an existing
llama-cpp>   directory.

quag avatar May 26 '25 20:05 quag