This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add mlir-vulkan-runner
ClosedPublic

Authored by denis13 on Jan 14 2020, 5:16 AM.

Details

Summary

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.

Diff Detail

Event Timeline

denis13 created this revision.Jan 14 2020, 5:16 AM
denis13 retitled this revision from [mlir][spirv][WIP] Vulkan runtime. to [mlir][spirv][WIP] mlir-vulkan-runner.Jan 14 2020, 5:21 AM

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!

flaub added a subscriber: flaub.Jan 28 2020, 5:13 PM
denis13 updated this revision to Diff 243661.Feb 10 2020, 1:09 PM
denis13 edited the summary of this revision. (Show Details)

Add mlir-vulkan-runner.
Add ConvertGpuLaunchFuncToVulkanCallsPass pass.
Add vulkan-runtime-wrapper C calls.
Add simple test.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2020, 1:09 PM

@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.

denis13 retitled this revision from [mlir][spirv][WIP] mlir-vulkan-runner to [mlir][spirv] mlir-vulkan-runner.Feb 10 2020, 1:17 PM
denis13 edited the summary of this revision. (Show Details)

@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

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).

denis13 added a comment.EditedFeb 11 2020, 2:58 AM

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:

  1. It creates llvm global with SPIR-V binary shader.
  2. It creates llvm global with entry point name.
  3. 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

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!

denis13 updated this revision to Diff 243897.EditedFeb 11 2020, 9:00 AM

Added simple test to check GpuLaunchFuncToVulkanCalssPass.
Rebased on D74012.
Rebased on 5a1778 which changes MemRef calling convention.

antiagainst requested changes to this revision.Feb 12 2020, 6:19 PM

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
10

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
11

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 ...

73

Insert an empty line and document this?

76

for each dimension of

84

Insert an empty line and document this?

87

Could we move the impl out side of the declaration like other large methods?

166

Print an errror saying "should only contain one 'spv.module' op"?

209

What about moving this to the end so that if we have errors we don't need to do this at all?

268

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
8

This line is a bit different. ;)

69

What about ResourceBufferBindingMap?

74

ResourceStorageClassBindingMap?

90

compute shader

93

SPIR-V

mlir/tools/mlir-vulkan-runner/mlir-vulkan-runner.cpp
29

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
11

We can delete this line given it's implementation details.

23

Wrap this class in anonymous namespace to hide?

29

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.

This revision now requires changes to proceed.Feb 12 2020, 6:19 PM

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.

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.

Thanks Denis! I've approved D74549. This one can be rebased against that and mark D74549 as a parent revision.

denis13 updated this revision to Diff 244534.Feb 13 2020, 1:57 PM

Addresses review comments.

denis13 updated this revision to Diff 244535.Feb 13 2020, 2:07 PM

Addresses review comments.

denis13 retitled this revision from [mlir][spirv] mlir-vulkan-runner to [mlir][spirv] Add mlir-vulkan-runner.Feb 13 2020, 2:10 PM
denis13 edited the summary of this revision. (Show Details)
denis13 updated this revision to Diff 244712.Feb 14 2020, 10:47 AM

Rebased on master to trigger a rebuild.

LGTM, but this should get @antiagainst's approval.
Thanks for the work! It'll be a valuable tool to have moving forward

Thanks, looking forward for the feedback from Lei.

antiagainst accepted this revision.Feb 19 2020, 7:11 AM

Awesome! Thanks again, Denis! This is very helpful!!

This revision is now accepted and ready to land.Feb 19 2020, 7:11 AM
This revision was automatically updated to reflect the committed changes.