This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add pass ConvertGpuLaunchFuncToVulkanCallsPass.
ClosedPublic

Authored by denis13 on Feb 13 2020, 5:16 AM.

Details

Summary

Implement a pass to convert gpu.launch_func op into a sequence of
Vulkan runtime calls. 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 manages
Vulkan runtime.

Diff Detail

Event Timeline

denis13 created this revision.Feb 13 2020, 5:16 AM
denis13 updated this revision to Diff 244401.Feb 13 2020, 5:23 AM

Fix typo in comment.

denis13 edited the summary of this revision. (Show Details)Feb 13 2020, 5:25 AM
denis13 updated this revision to Diff 244423.Feb 13 2020, 6:55 AM

Rebased on master.

antiagainst accepted this revision.Feb 13 2020, 8:03 AM

Nice, thanks Denis! Reviews already happened in D72696 so I just have two more nits here.

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

Move this into initializeCachedTypes or put all of them into the constructor of GpuLaunchFuncToVulkanCalssPass?

158

Could you document why we are putting a mysterious \0 in the middle here?

This revision is now accepted and ready to land.Feb 13 2020, 8:03 AM
denis13 updated this revision to Diff 244458.Feb 13 2020, 9:23 AM
denis13 marked an inline comment as done.

Addressed review comments.

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

I was looking how llvm.mlir globals lowering to llvm ir, and as far as I undertood in the current approach the AddNull is set to false
https://llvm.org/doxygen/classllvm_1_1ConstantDataArray.html#a3edef3fa47c611d3d10606591213e57b
, so we need to explicitly append \0 to align with C style string. Please fix me I'm wrong.

if (op.getValueOrNull()) {
     // String attributes are treated separately because they cannot appear as
     // in-function constants and are thus not supported by getLLVMConstant.
     if (auto strAttr = op.getValueOrNull().dyn_cast_or_null<StringAttr>()) {
       cst = llvm::ConstantDataArray::getString(
           llvmModule->getContext(), strAttr.getValue(), /*AddNull=*/false);
       type = cst->getType();
     } else {
       cst = getLLVMConstant(type, op.getValueOrNull(), op.getLoc());
     }
   } else if (Block *initializer = op.getInitialize
This revision was automatically updated to reflect the committed changes.
antiagainst added inline comments.Feb 13 2020, 11:11 AM
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp
158

Yeah I was just saying that we should document along the code. :)

rriddle added inline comments.Feb 13 2020, 11:14 AM
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp
83

SmallVectorImpl

156

SmallString ? That would remove the explicit StringRef conversion below.

162

Seems easier to just write (name + "_spv_entry_point_name").str()

175

Return the error directly it auto converts to failure.

179

Drop all of the trivial braces.

197

Same here.

210

///

@rriddle thanks, I'll address your comments D72696