-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Fixed cooperative matrix shaders on Vulkan builds when CROSS_COMPILE ON #2914
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.
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
Changes from all commits
04ab3d3
e6ab39f
62e5be8
eaa2808
3275375
3519d2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,36 @@ | ||
| find_package (Threads REQUIRED) | ||
| find_program(GLSLC_EXECUTABLE glslc) | ||
| if(NOT GLSLC_EXECUTABLE) | ||
| message(FATAL_ERROR "glslc not found.") | ||
|
|
||
| # Use glslc from the parent CMake context or find it on the host system | ||
| if(DEFINED Vulkan_GLSLC_EXECUTABLE) | ||
| set(GLSLC_EXECUTABLE ${Vulkan_GLSLC_EXECUTABLE}) | ||
| message(STATUS "Using glslc from parent CMake: ${GLSLC_EXECUTABLE}") | ||
| # Also define this at compile time to set the default value in the shader generator | ||
| add_compile_definitions(VULKAN_GLSLC_EXECUTABLE="${GLSLC_EXECUTABLE}") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I don't think we need this functionality at all. The vulkan-shaders-gen binary has a command-line switch which receives a path to the specified glslc. There is really no need to provide a statically compiled path to glslc, since it is already being passed in at runtime. |
||
| else() | ||
| # Find glslc on the host system, not the target | ||
| find_program(GLSLC_EXECUTABLE glslc NO_CMAKE_FIND_ROOT_PATH) | ||
| if(NOT GLSLC_EXECUTABLE) | ||
| message(FATAL_ERROR "glslc not found. Please set Vulkan_GLSLC_EXECUTABLE in parent CMake.") | ||
| endif() | ||
| message(STATUS "Found host glslc: ${GLSLC_EXECUTABLE}") | ||
| endif() | ||
|
|
||
| option(GGML_VULKAN_COOPMAT_GLSLC_SUPPORT "Enable coopmat shaders" OFF) | ||
| option(GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT "Enable coopmat2 shaders" OFF) | ||
|
|
||
| message(STATUS "GGML_VULKAN_COOPMAT_GLSLC_SUPPORT: ${GGML_VULKAN_COOPMAT_GLSLC_SUPPORT}") | ||
| message(STATUS "GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT: ${GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT}") | ||
|
|
||
| set(TARGET vulkan-shaders-gen) | ||
| add_executable(${TARGET} vulkan-shaders-gen.cpp) | ||
| if (GGML_VULKAN_COOPMAT_GLSLC_SUPPORT) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In ggml-org/llama.cpp#11695 (comment) I had guessed that we may need to pass through a cmake variable telling this makefile which version of glslc to use? Is that not necessary? It's just the #defines that weren't making it through?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not 100% sure if we need additional version to be used (and in my case it was working as same version as the host system was used). I can confirm that only providing the compile definitions is working for my build => https://github.com/sandrohanea/whisper.net/actions/runs/13977409227/job/39134498314 |
||
| target_compile_definitions(vulkan-shaders-gen PRIVATE GGML_VULKAN_COOPMAT_GLSLC_SUPPORT) | ||
| endif() | ||
|
|
||
| if (GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT) | ||
| target_compile_definitions(vulkan-shaders-gen PRIVATE GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT) | ||
| endif() | ||
|
|
||
| install(TARGETS ${TARGET} RUNTIME) | ||
| target_compile_features(${TARGET} PRIVATE cxx_std_17) | ||
| target_link_libraries(vulkan-shaders-gen PUBLIC Threads::Threads) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
|
|
||
|
|
||
|
|
||
| #include <iostream> | ||
| #include <fstream> | ||
| #include <sstream> | ||
|
|
@@ -35,7 +36,16 @@ | |
| std::mutex lock; | ||
| std::vector<std::pair<std::string, std::string>> shader_fnames; | ||
|
|
||
| std::string GLSLC = "glslc"; | ||
| // Default glslc path, can be overridden at compile time or runtime | ||
| #ifdef VULKAN_GLSLC_EXECUTABLE | ||
| // If VULKAN_GLSLC_EXECUTABLE is defined at compile time, use it | ||
| #define DEFAULT_GLSLC_EXECUTABLE VULKAN_GLSLC_EXECUTABLE | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See note above. I don't think there's any benefit to bake an executable path into the binary when it is already supplied as a command-line switch to vulkan-shaders-gen. |
||
| #else | ||
| // Otherwise default to "glslc" | ||
| #define DEFAULT_GLSLC_EXECUTABLE "glslc" | ||
| #endif | ||
|
|
||
| std::string GLSLC = DEFAULT_GLSLC_EXECUTABLE; | ||
| std::string input_dir = "vulkan-shaders"; | ||
| std::string output_dir = "/tmp"; | ||
| std::string target_hpp = "ggml-vulkan-shaders.hpp"; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what is the need for these Android checks? Perhaps I confused matters mentioning Android, but I believe it was working correctly with the feature detection—unless I am missing something. Feature detection by attempting to compile the shaders should be able to work on all systems.