Skip to content

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.

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

Closed
sandrohanea wants to merge 6 commits into from
Closed

Fixed cooperative matrix shaders on Vulkan builds when CROSS_COMPILE ON #2914

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
04ab3d3
Fixed COOPMAT on Vulkan builds with CROSS_COMPILE ON
Mar 20, 2025
e6ab39f
Fixed a warning to retrigger CI (failed transiently last time)
Mar 21, 2025
62e5be8
Simplified the condition to just pass the DGGML_VULKAN_COOPMAT_GLSLC_…
Mar 26, 2025
eaa2808
Add status messages for GL_NV_cooperative_matrix2 support in Vulkan
Mar 26, 2025
3275375
Enhance Vulkan CMake configuration to allow explicit glslc executable…
Mar 26, 2025
3519d2d
WIP Refactor Vulkan CMake configuration to improve glslc executable h…
Mar 26, 2025
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
21 changes: 18 additions & 3 deletions ggml/src/ggml-vulkan/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@ cmake_policy(SET CMP0114 NEW)

find_package(Vulkan COMPONENTS glslc REQUIRED)

# Allow explicitly specifying glslc executable for cross-compilation scenarios
if(DEFINED GGML_VULKAN_GLSLC_EXECUTABLE)

@mtmcp mtmcp Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not convinced that providing a command-line override is the right solution here. When cross-compiling the standard behavior is to separate build/target utilities via the find-root path. For example, the line above is stating that it requires glslc, but it seems this should not in fact be required but found separately using a appropriate paths.

For example, what we are basically saying is:

  1. We need to find Vulkan as defined in the cross-compile toolchain.
  2. We need the build system glslc no matter what.

As such, it would be more consistent with general cross-compiling to instead use find_program(VULKAN_GLSLC_EXECUTABLE glslc REQUIRED NO_CMAKE_FIND_ROOT_PATH) and change the Vulkan find command to find_package(Vulkan) (not specifying the glslc component there).

@mtmcp mtmcp Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking about the Android case though, where there are multiple NDKs, and each might have its own glslc might be worthwhile to support both modes. So, the default behavior could be to use find_program, but also allow the command-line option to ensure we have our bases covered. There have been a couple issues surrounding this so probably best to play it safe.

@mtmcp mtmcp Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay, so I just took a quick look in the FindVulkan.cmake file on my system and quickly checked the docs. Based on the docs, the search for glslc will not be repeated if it is cached first:

This command is used to find a program. A cache entry, or a normal variable if NO_CACHE is specified, named by is created to store the result of this command. If the program is found the result is stored in the variable and the search will not be repeated unless the variable is cleared. If nothing is found, the result will be -NOTFOUND.

One possibility is to set the cache variable Vulkan_GLSLC_EXECUTABLE before calling find_package(Vulkan COMPONENTS glslc REQUIRED). So, first running find_program(Vulkan_GLSLC_EXECUTABLE glslc REQUIRED NO_CMAKE_FIND_ROOT_PATH) would override the search. Similarly, the command-line approach overriding the Vulkan_GLSLC_EXECUTABLE cache variable would override that, and then we don't need a separate variable for the same thing. Something to consider. 🙂

@jeffbolznv jeffbolznv Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking about the Android case though, where there are multiple NDKs, and each might have its own glslc might be worthwhile to support both modes.

IMO we should just always use the glslc installed on the build system. The NDK glslc is egregiously out of date, it's really not what we want to be using.

@sandrohanea sandrohanea Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated the description of the PR, let me know what do you think

set(VULKAN_GLSLC_EXECUTABLE_OVERRIDE ${GGML_VULKAN_GLSLC_EXECUTABLE})
message(STATUS "Using explicitly provided glslc: ${VULKAN_GLSLC_EXECUTABLE_OVERRIDE}")
else()
set(VULKAN_GLSLC_EXECUTABLE_OVERRIDE ${Vulkan_GLSLC_EXECUTABLE})
endif()

function(detect_host_compiler)
if (CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows")
find_program(HOST_C_COMPILER NAMES cl gcc clang NO_CMAKE_FIND_ROOT_PATH)
Expand All @@ -26,28 +34,32 @@ if (Vulkan_FOUND)
# Compile a test shader to determine whether GL_KHR_cooperative_matrix is supported.
# If it's not, there will be an error to stderr.
# If it's supported, set a define to indicate that we should compile those shaders
execute_process(COMMAND ${Vulkan_GLSLC_EXECUTABLE} -o - -fshader-stage=compute --target-env=vulkan1.3 "${CMAKE_CURRENT_SOURCE_DIR}/vulkan-shaders/test_coopmat_support.comp"
execute_process(COMMAND ${VULKAN_GLSLC_EXECUTABLE_OVERRIDE} -o - -fshader-stage=compute --target-env=vulkan1.3 "${CMAKE_CURRENT_SOURCE_DIR}/vulkan-shaders/test_coopmat_support.comp"
OUTPUT_VARIABLE glslc_output
ERROR_VARIABLE glslc_error)

if (${glslc_error} MATCHES ".*extension not supported: GL_KHR_cooperative_matrix.*")
message(STATUS "GL_KHR_cooperative_matrix not supported by glslc")
set(GGML_VULKAN_COOPMAT_GLSLC_SUPPORT OFF CACHE INTERNAL "Not enable coopmat shaders")
else()
message(STATUS "GL_KHR_cooperative_matrix supported by glslc")
add_compile_definitions(GGML_VULKAN_COOPMAT_GLSLC_SUPPORT)
set(GGML_VULKAN_COOPMAT_GLSLC_SUPPORT ON CACHE INTERNAL "Enable coopmat shaders")
endif()

# Compile a test shader to determine whether GL_NV_cooperative_matrix2 is supported.
# If it's not, there will be an error to stderr.
# If it's supported, set a define to indicate that we should compile those shaders
execute_process(COMMAND ${Vulkan_GLSLC_EXECUTABLE} -o - -fshader-stage=compute --target-env=vulkan1.3 "${CMAKE_CURRENT_SOURCE_DIR}/vulkan-shaders/test_coopmat2_support.comp"
execute_process(COMMAND ${VULKAN_GLSLC_EXECUTABLE_OVERRIDE} -o - -fshader-stage=compute --target-env=vulkan1.3 "${CMAKE_CURRENT_SOURCE_DIR}/vulkan-shaders/test_coopmat2_support.comp"
OUTPUT_VARIABLE glslc_output
ERROR_VARIABLE glslc_error)

if (${glslc_error} MATCHES ".*extension not supported: GL_NV_cooperative_matrix2.*")
message(STATUS "GL_NV_cooperative_matrix2 not supported by glslc")

@jeffbolznv jeffbolznv Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should there be an "OFF" here like there is for coopmat1 above?

@sandrohanea sandrohanea Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch!
I got too fast on it, thanks for catching it!

It would not be consistent otherwise => won't cause issues as default will be OFF anyway, but I think it is better to have it explicit.

set(GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT OFF CACHE INTERNAL "Not enable coopmat2 shaders")
else()
message(STATUS "GL_NV_cooperative_matrix2 supported by glslc")
set(GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT ON CACHE INTERNAL "Enable coopmat2 shaders")
add_compile_definitions(GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT)
endif()

Expand Down Expand Up @@ -119,6 +131,9 @@ if (Vulkan_FOUND)
SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/vulkan-shaders
CMAKE_ARGS -DCMAKE_TOOLCHAIN_FILE=${HOST_CMAKE_TOOLCHAIN_FILE}
-DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}
-DGGML_VULKAN_COOPMAT_GLSLC_SUPPORT=${GGML_VULKAN_COOPMAT_GLSLC_SUPPORT}
-DGGML_VULKAN_COOPMAT2_GLSLC_SUPPORT=${GGML_VULKAN_COOPMAT2_GLSLC_SUPPORT}
-DGGML_VULKAN_GLSLC_EXECUTABLE=${VULKAN_GLSLC_EXECUTABLE_OVERRIDE}
BUILD_COMMAND ${CMAKE_COMMAND} --build .
INSTALL_COMMAND ${CMAKE_COMMAND} --install .
INSTALL_DIR ${CMAKE_BINARY_DIR}
Expand All @@ -144,7 +159,7 @@ if (Vulkan_FOUND)
${_ggml_vk_source}

COMMAND ${_ggml_vk_genshaders_cmd}
--glslc ${Vulkan_GLSLC_EXECUTABLE}
--glslc ${VULKAN_GLSLC_EXECUTABLE_OVERRIDE}
--input-dir ${_ggml_vk_input_dir}
--output-dir ${_ggml_vk_output_dir}
--target-hpp ${_ggml_vk_header}
Expand Down
2 changes: 1 addition & 1 deletion ggml/src/ggml-vulkan/ggml-vulkan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1394,7 +1394,7 @@ static std::array<uint32_t, 2> fa_rows_cols(uint32_t D, uint32_t clamp, ggml_typ
return {64, 32};
}
return {64, 64};
};
}

static bool ggml_vk_matmul_shmem_support(const vk_device& device, const std::vector<uint32_t>& warptile, bool mul_mat_id, ggml_type src0_type) {

Expand Down
27 changes: 24 additions & 3 deletions ggml/src/ggml-vulkan/vulkan-shaders/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,32 @@
find_package (Threads REQUIRED)
find_program(GLSLC_EXECUTABLE glslc)
if(NOT GLSLC_EXECUTABLE)
message(FATAL_ERROR "glslc not found.")

# Allow explicitly specifying glslc executable path from parent CMake
if(DEFINED GGML_VULKAN_GLSLC_EXECUTABLE)
set(GLSLC_EXECUTABLE ${GGML_VULKAN_GLSLC_EXECUTABLE})
message(STATUS "Using explicitly provided glslc: ${GLSLC_EXECUTABLE}")
else()
find_program(GLSLC_EXECUTABLE glslc)
if(NOT GLSLC_EXECUTABLE)
message(FATAL_ERROR "glslc not found.")
endif()
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)

@jeffbolznv jeffbolznv Mar 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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?

@sandrohanea sandrohanea Mar 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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)
8 changes: 7 additions & 1 deletion ggml/src/ggml-vulkan/vulkan-shaders/vulkan-shaders-gen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@



#include <iostream>
#include <fstream>
#include <sstream>
Expand Down Expand Up @@ -35,7 +36,12 @@
std::mutex lock;
std::vector<std::pair<std::string, std::string>> shader_fnames;

std::string GLSLC = "glslc";
// Default glslc path, can be overridden by --glslc command line argument
#ifndef VULKAN_GLSLC_EXECUTABLE
#define VULKAN_GLSLC_EXECUTABLE "glslc"
#endif

std::string GLSLC = VULKAN_GLSLC_EXECUTABLE;
std::string input_dir = "vulkan-shaders";
std::string output_dir = "/tmp";
std::string target_hpp = "ggml-vulkan-shaders.hpp";
Expand Down