Skip to content
Merged
Changes from all commits
Commits
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
43 changes: 43 additions & 0 deletions ggml/src/ggml-vulkan/ggml-vulkan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,14 @@
#include "ggml-cpu.h"
#endif

// See https://github.com/KhronosGroup/Vulkan-Hpp?tab=readme-ov-file#extensions--per-device-function-pointers-
#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1

#include <vulkan/vulkan.hpp>

// See https://github.com/KhronosGroup/Vulkan-Hpp?tab=readme-ov-file#extensions--per-device-function-pointers-
VULKAN_HPP_DEFAULT_DISPATCH_LOADER_DYNAMIC_STORAGE

Comment on lines +8 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this doesn't belong into the ggml library. It can exist once in the whole program.
ggml-org/ggml#1344 (comment).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let me see if I can forgo the macros and define a dispatcher with static linkage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't get static to work - vulkan-hpp uses extern to forward declare and it can't be both extern and static.

Choosing a unique namespace ought to work. @dg0yt please try this:

diff --git a/ggml/src/ggml-vulkan/ggml-vulkan.cpp b/ggml/src/ggml-vulkan/ggml-vulkan.cpp
index 0feaf4cb5..bc26ec8f5 100644
--- a/ggml/src/ggml-vulkan/ggml-vulkan.cpp
+++ b/ggml/src/ggml-vulkan/ggml-vulkan.cpp
@@ -5,6 +5,9 @@
 #include "ggml-cpu.h"
 #endif

+// make vulkan-hpp vk namespace unique for this file, to avoid conflicts when linking
+#define vk ggml_vk
+
 // See https://github.com/KhronosGroup/Vulkan-Hpp?tab=readme-ov-file#extensions--per-device-function-pointers-
 #define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1

Copy link
Collaborator

Choose a reason for hiding this comment

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

Possible alternative that only renames the dispatcher:

namespace vk::detail { class DispatchLoaderDynamic; }
vk::detail::DispatchLoaderDynamic & ggml_vk_default_dispatcher();

#define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1
#define VULKAN_HPP_DEFAULT_DISPATCHER ggml_vk_default_dispatcher()

#include <vulkan/vulkan.hpp>

static vk::detail::DispatchLoaderDynamic ggml_vk_default_dispatcher_instance;
vk::detail::DispatchLoaderDynamic & ggml_vk_default_dispatcher() {
    return ggml_vk_default_dispatcher_instance;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh yeah, laundering it through a function call may work.

Copy link
Contributor

Choose a reason for hiding this comment

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

With recent vulkan-hpp, it would be a one-line change, but for my installation I had to deal with KhronosGroup/Vulkan-Hpp#2093.

Scratch that 🤦 I must have tested with a different patch. The macro is good, but the workaround isn't, and then there is vk:: in ggml-vulkan.cpp.

This does work:

diff --git a/src/ggml-vulkan/ggml-vulkan.cpp b/src/ggml-vulkan/ggml-vulkan.cpp
index 5c941e72..a98f29e7 100644
--- a/src/ggml-vulkan/ggml-vulkan.cpp
+++ b/src/ggml-vulkan/ggml-vulkan.cpp
@@ -8,6 +8,9 @@
 // See https://github.com/KhronosGroup/Vulkan-Hpp?tab=readme-ov-file#extensions--per-device-function-pointers-
 #define VULKAN_HPP_DISPATCH_LOADER_DYNAMIC 1
 
+#define VULKAN_HPP_NAMESPACE ggml_vk
+namespace ggml_vk {}
+namespace vk { using namespace ::ggml_vk; }
 #include <vulkan/vulkan.hpp>
 
 

Choose a reason for hiding this comment

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

Can one of you make a PR and discuss it on there?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer @Acly's fix. @Acly do you want to make a PR?

Choose a reason for hiding this comment

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

Thanks! please make a link to PR or just tag me there. 🙏🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, PR is here: #16224

#include <algorithm>
#include <cmath>
#include <iomanip>
Expand Down Expand Up @@ -121,6 +127,8 @@ struct vk_pipeline_struct {
bool needed {};
// set to true when the shader has been compiled
bool compiled {};
// number of registers used, extracted from pipeline executable properties
uint32_t register_count {};
};

typedef std::shared_ptr<vk_pipeline_struct> vk_pipeline;
Expand Down Expand Up @@ -429,6 +437,8 @@ struct vk_device_struct {

bool coopmat2;

bool pipeline_executable_properties_support {};

size_t idx;

bool mul_mat_l[GGML_TYPE_COUNT];
Expand Down Expand Up @@ -1519,6 +1529,20 @@ static void ggml_vk_create_pipeline_func(vk_device& device, vk_pipeline& pipelin
vk_instance.pfn_vkSetDebugUtilsObjectNameEXT(device->device, &static_cast<VkDebugUtilsObjectNameInfoEXT &>(duoni));
}

if (device->pipeline_executable_properties_support) {
vk::PipelineExecutableInfoKHR executableInfo;
executableInfo.pipeline = pipeline->pipeline;

auto statistics = device->device.getPipelineExecutableStatisticsKHR(executableInfo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Debug mode is now giving me this warning:

vkGetPipelineExecutableStatisticsKHR(): called on a pipeline created without the VK_PIPELINE_CREATE_CAPTURE_STATISTICS_BIT_KHR flag set.
The Vulkan spec states: The pipeline member of pExecutableInfo must have been created with VK_PIPELINE_CREATE_CAPTURE_STATISTICS_BIT_KHR (https://docs.vulkan.org/spec/latest/chapters/pipelines.html#VUID-vkGetPipelineExecutableStatisticsKHR-pipeline-03274)

for (auto & s : statistics) {
// "Register Count" is reported by NVIDIA drivers.
if (strcmp(s.name, "Register Count") == 0) {
VK_LOG_DEBUG(pipeline->name << " " << s.name << ": " << s.value.u64 << " registers");
pipeline->register_count = (uint32_t)s.value.u64;
}
}
}

{
std::lock_guard<std::recursive_mutex> guard(device->mutex);
device->all_pipelines.push_back(pipeline);
Expand Down Expand Up @@ -3509,6 +3533,7 @@ static vk_device ggml_vk_get_device(size_t idx) {
bool amd_shader_core_properties2 = false;
bool pipeline_robustness = false;
bool coopmat2_support = false;
bool pipeline_executable_properties_support = false;
device->coopmat_support = false;
device->integer_dot_product = false;
bool bfloat16_support = false;
Expand Down Expand Up @@ -3551,6 +3576,8 @@ static vk_device ggml_vk_get_device(size_t idx) {
!getenv("GGML_VK_DISABLE_BFLOAT16")) {
bfloat16_support = true;
#endif
} else if (strcmp("VK_KHR_pipeline_executable_properties", properties.extensionName) == 0) {
pipeline_executable_properties_support = true;
}
}

Expand Down Expand Up @@ -3771,8 +3798,18 @@ static vk_device ggml_vk_get_device(size_t idx) {
device_extensions.push_back("VK_KHR_shader_integer_dot_product");
}

VkPhysicalDevicePipelineExecutablePropertiesFeaturesKHR pep_features {};
pep_features.sType = VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_PIPELINE_EXECUTABLE_PROPERTIES_FEATURES_KHR;
if (pipeline_executable_properties_support) {
last_struct->pNext = (VkBaseOutStructure *)&pep_features;
last_struct = (VkBaseOutStructure *)&pep_features;
device_extensions.push_back("VK_KHR_pipeline_executable_properties");
}

vkGetPhysicalDeviceFeatures2(device->physical_device, &device_features2);

device->pipeline_executable_properties_support = pipeline_executable_properties_support;

device->fp16 = device->fp16 && vk12_features.shaderFloat16;

#if defined(VK_KHR_shader_bfloat16)
Expand Down Expand Up @@ -4288,6 +4325,9 @@ static void ggml_vk_instance_init() {
}
VK_LOG_DEBUG("ggml_vk_instance_init()");

// See https://github.com/KhronosGroup/Vulkan-Hpp?tab=readme-ov-file#extensions--per-device-function-pointers-
VULKAN_HPP_DEFAULT_DISPATCHER.init(vkGetInstanceProcAddr);

uint32_t api_version = vk::enumerateInstanceVersion();

if (api_version < VK_API_VERSION_1_2) {
Expand Down Expand Up @@ -4355,6 +4395,9 @@ static void ggml_vk_instance_init() {

vk_perf_logger_enabled = getenv("GGML_VK_PERF_LOGGER") != nullptr;

// See https://github.com/KhronosGroup/Vulkan-Hpp?tab=readme-ov-file#extensions--per-device-function-pointers-
VULKAN_HPP_DEFAULT_DISPATCHER.init(vk_instance.instance);

std::vector<vk::PhysicalDevice> devices = vk_instance.instance.enumeratePhysicalDevices();

// Emulate behavior of CUDA_VISIBLE_DEVICES for Vulkan
Expand Down