Add an initial version of mlir-vulkan-runner execution driver.
A command line utility that executes a MLIR file on the Vulkan by
translating MLIR GPU module to SPIR-V and host part to LLVM IR before
JIT-compiling and executing the latter.
Details
Diff Detail
Event Timeline
Hi is this ready for review? Or actually is it just a re-export of the previous PR on GitHub?
@antiagainst sorry for dealy in responce, missed the comment somehow, I've just moved it from github repo and updated according the latest comments. I've started to implement mlir-vulkan-runner tool to make it simliar to mlir-cuda-runner. Thanks!
Add mlir-vulkan-runner.
Add ConvertGpuLaunchFuncToVulkanCallsPass pass.
Add vulkan-runtime-wrapper C calls.
Add simple test.
@antiagainst @mravishankar this is an initial version of mlir-vulkan-runner. It's now possible to execute GPU code on Vulkan capable devices via JitRunner.
I'll review this tomorrow; but wanted to say this is super awesome! Thanks a ton for all the great work, Denis! Really appreciate it. :D
Nice work! Glad to see this coming together :)
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp | ||
---|---|---|
27 ↗ | (On Diff #243661) | This pass does not seem to include anything that requires a Vulkan environment, it seems like it could be tested (and landed) independently, and not gated in CMake by the runner being enabled (which will help keeping this building in CI). |
Thanks for looking on this. The VulkanRuntime part is not changed since the last time. This PR covers that was discussed when MLIR was in Tensorflow repo. The instrumentation pass is simple as possible:
- It creates llvm global with SPIR-V binary shader.
- It creates llvm global with entry point name.
- It creates 4 runtime calls: setBinaryShader, setEntryPoint, setNumWorkGroups, runOnVulkan.
Since the MLIR does not define any runtime and this part is only for integration testing, I hope, at this moment, this runtime API is enough. By the way, the runtime API could be improved gradually if needed, the dispatching and command buffer creation could be introduced by instrumented runtime calls as well, the current approach hides inside all Vulkan runtime machinery like command buffer creation, pipeline creation, dispatching and so on. (Vulkan API is huge even to bootstrap computation pipeline).
Thanks!
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp | ||
---|---|---|
27 ↗ | (On Diff #243661) | Yes, it only requires LLVM Dialect and spirv::Serializer, the tests are missing for this pass as well. I actually was thinking to introduce simple working pipeline at the first iteration and if needed I can split the pass which instrument Vulkan runtime calls to the host part and Vulkan runtime to separate PRs. Thanks! |
Added simple test to check GpuLaunchFuncToVulkanCalssPass.
Rebased on D74012.
Rebased on 5a1778 which changes MemRef calling convention.
Super nice! This is so great! Thanks a ton, Denis! I don't have major concerns given that a large portion of this patch has already went through reviews multiple times previously. Just a few nits. We can land and then iterate on it. :)
mlir/include/mlir/Conversion/GPUToVulkan/ConvertGPUToVulkanPass.h | ||
---|---|---|
9 ↗ | (On Diff #243897) | Nit: "This file declares passes to convert GPU dialect ops to Vulkan ..." so later if we add more entry points we don't need to update this. |
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp | ||
10 ↗ | (On Diff #243897) | The Vulkan runtime API surface is huge so currently we don't expose separate external functions in IR for each of them, instead we expose a few external functions to wrapper libraries which ... |
72 ↗ | (On Diff #243897) | Insert an empty line and document this? |
75 ↗ | (On Diff #243897) | for each dimension of |
83 ↗ | (On Diff #243897) | Insert an empty line and document this? |
86 ↗ | (On Diff #243897) | Could we move the impl out side of the declaration like other large methods? |
165 ↗ | (On Diff #243897) | Print an errror saying "should only contain one 'spv.module' op"? |
208 ↗ | (On Diff #243897) | What about moving this to the end so that if we have errors we don't need to do this at all? |
267 ↗ | (On Diff #243897) | gpu.launch_func |
mlir/test/Conversion/GPUToVulkan/simple.mlir | ||
1 ↗ | (On Diff #243897) | Can we have a more descriptive name? simple.mlir does not actually say anything. What about invoke-vulkan.mlir? |
14 ↗ | (On Diff #243897) | Super nit: just call it kernel would be fine. The _1 does not provide additional value. |
mlir/tools/mlir-vulkan-runner/VulkanRuntime.cpp | ||
58 | Print an error saying "unsupported storage class"? | |
73 | Same here | |
85 | Print an error? | |
134 | Move to the beginning to be safe? | |
646 | Prefer to use nullptr for pointer fields. | |
670 | = {} | |
mlir/tools/mlir-vulkan-runner/VulkanRuntime.h | ||
7 ↗ | (On Diff #243897) | This line is a bit different. ;) |
68 ↗ | (On Diff #243897) | What about ResourceBufferBindingMap? |
73 ↗ | (On Diff #243897) | ResourceStorageClassBindingMap? |
89 ↗ | (On Diff #243897) | compute shader |
92 ↗ | (On Diff #243897) | SPIR-V |
mlir/tools/mlir-vulkan-runner/mlir-vulkan-runner.cpp | ||
28 ↗ | (On Diff #243897) | Let's name it as module. m and other single char names should really have a very small scope. |
mlir/tools/mlir-vulkan-runner/vulkan-runtime-wrappers.cpp | ||
10 ↗ | (On Diff #243897) | We can delete this line given it's implementation details. |
22 ↗ | (On Diff #243897) | Wrap this class in anonymous namespace to hide? |
28 ↗ | (On Diff #243897) | Hmm, instead of having this singleton, what about exposing two more functions, initVulkan and deinitVulkan to be more explicit? initVulkan can return a pointer that gets passed into other functions like setResourceData, etc. This file will be compiled into libraries so having this kind of static stuff is not friendly. I'm fine to use llvm::ManagedStatic for now to avoid huge changes again given this patch is already fair large and has been waited for a long time. We can put a TODO here and add initVulkan and deinitVulkan later. |
I’d rather see the pass extracted in a separate parent revision and tested independently.
Thanks
Thanks a lot for review! I addressed comments related to pass in separate revision https://reviews.llvm.org/D74549 and will update mlir-vulkan-runner, VulkanRuntime as well.
LGTM, but this should get @antiagainst's approval.
Thanks for the work! It'll be a valuable tool to have moving forward
Print an error saying "unsupported storage class"?