This is an archive of the discontinued LLVM Phabricator instance.

[mlir][GPUToVulkan] Port conversion passes and `mlir-vulkan-runner` to opaque pointers
ClosedPublic

Authored by zero9178 on Feb 21 2023, 1:57 AM.

Details

Summary

Part of https://discourse.llvm.org/t/rfc-switching-the-llvm-dialect-and-dialect-lowerings-to-opaque-pointers/68179

This patch adds the new pass option 'use-opaque-pointers' to -launch-func-to-vulkan instructing the pass to emit LLVM opaque pointers instead of typed pointers.

Note that the pass as it was previously implemented relied on the fact LLVM pointers carried an element type. The passed used this information to deduce both the rank of a "lowered-to-llvm" MemRef as well as the element type. Since the element type when using LLVM opaque pointers is completely erased it is not possible to deduce the element type.

I therefore added a new attribute that is attached to the vulkanLaunch call alongside the binary blob and entry point name by the -convert-gpu-launch-to-vulkan-launch pass. It simply attaches a type array specifying the element types of each memref. This way the -launch-func-to-vulkan can simply read out the element type from the attribute.
The rank can still be deduced from the auto-generated C interface from FinalizeMemRefToLLVM. This is admittedly a bit fragile but I was not sure whether it was worth the effort to also add a rank array attribute.

As a last step, the use of opaque-pointers in mlir-vulkan-runners codegen pipeline was also enabled, since all covnersion passes used fully support it.

Diff Detail

Event Timeline

zero9178 created this revision.Feb 21 2023, 1:57 AM
zero9178 requested review of this revision.Feb 21 2023, 1:57 AM
gysit added a subscriber: gysit.Feb 22 2023, 4:05 AM

LGTM!

Do we want to switch the mlir-vulkan-runner already in this commit or should this be a followup commit?

@ThomasRaoux or @antiagainst may want to have a second look since I am not very familiar with that part of the code base.

mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp
174

Is the blob the actual compiled kernel? If yes, what about calling it binary, shaderBinary, kernelBinary or similar? Or, is blob SPIRV-terminology.

309

nit: I would use braces here since especially the else block is quite large.

zero9178 updated this revision to Diff 499656.Feb 22 2023, 3:07 PM

Address review comments: braces around larger else body

zero9178 marked an inline comment as done.Feb 22 2023, 3:11 PM

Do we want to switch the mlir-vulkan-runner already in this commit or should this be a followup commit?

Don't feel tooo strongly about this, but I'd prefer to do this in this commit. The way I see it, because mlir-vulkan-runner is the only consumer of these passes in-repo, switching it as part of this commit adds extra test coverage for free.

mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp
174

Yes, but I'd like to avoid using another word for the same thing (aka I want to stay consistent with the existing terminlogy). I think blob as in "binary blob" also makes sense.

Don't feel tooo strongly about this, but I'd prefer to do this in this commit. The way I see it, because mlir-vulkan-runner is the only consumer of these passes in-repo, switching it as part of this commit adds extra test coverage for free.

Ok let's land as is then!

zero9178 updated this revision to Diff 499765.Feb 23 2023, 1:19 AM
zero9178 marked an inline comment as done.

clang-format

antiagainst accepted this revision.Feb 24 2023, 8:09 AM

Thanks for plumbing this through!

This revision is now accepted and ready to land.Feb 24 2023, 8:09 AM