vulkan : cmake integration #8119
Conversation
mofosyne
added
the
Review Complexity : Low
github-actions
bot
added
build
|
The |
|
It's worth noting here that this will add |
github-actions
bot
added
the
devops
|
I agree the Vulkan SDK is somewhat heavy dependency and pulls in lots of graphics-related dependencies. For Arch and MSYS2 there are the |
github-actions
bot
added
the
nix
Yes, I understand it, and that's what I'm hesitant about: w64devkit is about portability - which is lost in currently suggested workflow from Readme. I think "copy folders + create a correct vulkan.pc file" should be preferable for that reason: Windows (w64devkit)Download w64devkit and unpack it into a preferable folder. Run w64devkit.exe, switch into |
|
Understood—I'll add those steps in the readme. 😊 |
github-actions
bot
added
the
documentation
This is FYI only (for now):I'm trying to get llama.cpp to work with Vulkan on SnapDragon X Copilot+PCs on Windows for Arm. I installed the Vulkan-SDK for Windows (x64). Then I built Vulkan-Loader's vulkan1.lib as MSVC Windows arm64 .lib and replaced the Vulkan SDK's x64 lib with it. The main llama.cpp branch seems to build OK with 'cmake -B build -DGGML_VULKAN=1' (on MSVC), but it does not work. I will try and maybe find/fix the issue with the main branch, before trying to test your PR. I will update you, when I have better results. But any ideas very welcome. llama.cpp llama-cli log: [1720599200] Log start Windows vulkaininfoSDK --summary: Vulkan Instance Version: 1.3.277 Instance Extensions: count = 13 Instance Layers: count = 10 Devices: GPU0: GPU1: GPU2: |
|
@AndreasKunar That's great, can you open an issue about that and track your progress/problems there? @bandoti I'm sorry I haven't managed to review this yet, I don't have much time at the moment. I'll try to do it this evening. |
Thanks! I will first try and debug the vulkan1.lib, to make sure that it works (with the Vulkan samples). Once I know, that my WoA vulkan1.lib is OK, I will open an issue (probably not before late tomorrow). |
There was a problem hiding this comment.
Looks good to me. I tested it and it works perfectly, on Linux.
What I noticed that if you have Vulkan, but not glslc, you get a lot of spam like this:
cannot compile matmul_f32_f16_fp32
Vulkan_GLSLC_EXECUTABLE-NOTFOUND -fshader-stage=compute --target-env=vulkan1.2 -O /home/user/llama.cpp/ggml/src/vulkan-shaders/mul_mm.comp -o /home/user/llama.cpp/build_vk/ggml/src/vulkan-shaders.spv/matmul_f32_f16_fp32.spv -DB_TYPE=float16_t -DDATA_A_F32=1 -DD_TYPE=float -DFLOAT_TYPE=float
sh: 1: Vulkan_GLSLC_EXECUTABLE-NOTFOUND: not found
It should be possible to have it throw an error earlier if the shader compiler isn't found, right?
|
Ah—yeah, in that case we should probably add those deps to find_package(Vulkan COMPONENTS glslc REQUIRED). I'll get that change in shortly. In a future pull request we can add checks for MoltenVk to get it working on Apple too. Would rather defer that though to get the broader change in there first. 😊 |
|
Why would MoltenVK not work with these changes? It was working before. |
|
I'd imagine it would work still! Just haven't tested it myself. I guess more specifically I meant in the find_package it can check for MoltenVk and cause an error if it's not found. However, I don't think that'll be an issue because it's kind of assumed on MacOS that this will be installed with Vulkan (unless it's an intel mac I guess). |
|
@ggerganov Considering this touches quite a few files beyond my Vulkan backend, can this be merged, or is there something we should wait for (like another PR or someone else's approval)? |
|
I tested your scripts on Windows for Arm.
This is NOT because of your cmake/vulkan but because of a recent issue with ggml-aarch64.c raised by me with issue#8446 - they do not check for MSVC correctly and generate a gcc _asm_ directive, which aborts the compile. If I fix ggml-aarch64.c with the correct MSVC-excluding compiler-conditionals, you also build for MSVC.
|
There was a problem hiding this comment.
@0cc4m It's good to merge 👍
@AndreasKunar The Arm issue is noted and will be resolved in the scope of #8446
| get_directory_property(GGML_DIR_DEFINES DIRECTORY ggml/src COMPILE_DEFINITIONS) | ||
| get_target_property(GGML_TARGET_DEFINES ggml COMPILE_DEFINITIONS) | ||
| set(GGML_TRANSIENT_DEFINES ${GGML_TARGET_DEFINES} ${GGML_DIR_DEFINES}) | ||
| get_target_property(GGML_LINK_LIBRARIES ggml LINK_LIBRARIES) |
There was a problem hiding this comment.
I don't understand why this is needed - an example with a specific flag that is not exported would help. It's OK to merge as it is though
There was a problem hiding this comment.
@ggerganov I will take a closer look at this. The flags which should be exported should be all interface defines (public interface only) on the ggml/llama libraries. I will follow up with a new pull request.
* Add Vulkan to CMake pkg * Add Sycl to CMake pkg * Add OpenMP to CMake pkg * Split generated shader file into separate translation unit * Add CMake target for Vulkan shaders * Update README.md * Add make target for Vulkan shaders * Use pkg-config to locate vulkan library * Add vulkan SDK dep to ubuntu-22-cmake-vulkan workflow * Clean up tabs * Move sudo to apt-key invocation * Forward GGML_EXTRA_LIBS to CMake config pkg * Update vulkan obj file paths * Add shaderc to nix pkg * Add python3 to Vulkan nix build * Link against ggml in cmake pkg * Remove Python dependency from Vulkan build * code review changes * Remove trailing newline * Add cflags from pkg-config to fix w64devkit build * Update README.md * Remove trailing whitespace * Update README.md * Remove trailing whitespace * Fix doc heading * Make glslc required Vulkan component * remove clblast from nix pkg
This change introduces a
makeandCMakebuild target for Vulkan shaders per #5356. This ensuresggml-vulkan-shaders.hppis generated at build time (instead of storing in SCM). In addition,ggml-vulkan-shaders.cppis added to move compiled shaders into its own translation unit.In addition, this change updates the relocatable CMake package to link against the new
ggmllibrary.