-
Notifications
You must be signed in to change notification settings - Fork 13.4k
vulkan: initialize vulkan-hpp to allow using extension function pointers #15705
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
Conversation
Use this to query register count for shader compiles on NVIDIA. Currently this is only for performance debug, but it could eventually be used in some heuristics like split_k.
818a813 to
90e726e
Compare
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.
Sorry for the delay
| vk::PipelineExecutableInfoKHR executableInfo; | ||
| executableInfo.pipeline = pipeline->pipeline; | ||
|
|
||
| auto statistics = device->device.getPipelineExecutableStatisticsKHR(executableInfo); |
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.
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)
|
I think this report is related to the changes in this PR: ggml-org/ggml#1344 (comment) |
| // 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 | ||
|
|
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.
IMO this doesn't belong into the ggml library. It can exist once in the whole program.
ggml-org/ggml#1344 (comment).
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.
Let me see if I can forgo the macros and define a dispatcher with static linkage.
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 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
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.
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;
}
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.
oh yeah, laundering it through a function call may work.
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.
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>
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.
Can one of you make a PR and discuss it on there?
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.
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.
Thanks! please make a link to PR or just tag me there. 🙏🏻
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.
Ok, PR is here: #16224
Use this to query register count for shader compiles on NVIDIA. Currently this is only for performance debug, but it could eventually be used in some heuristics like split_k.
Fixes #15698.