This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vulkan-runner] Use C-compatible wrapper emission.
ClosedPublic

Authored by denis13 on Mar 12 2020, 5:07 AM.

Details

Summary

A memref argument is converted into a pointer-to-struct argument
of type {T*, T*, i64, i64[N], i64[N]}* in the wrapper function,
where T is the converted element type and N is the memref rank.

Diff Detail

Event Timeline

denis13 created this revision.Mar 12 2020, 5:07 AM
Herald added a project: Restricted Project. · View Herald Transcript

This patch eliminates VulkanLaunchOpOperandAdaptor and enables emitCWrappers while lowering to LLVM dialect. By doing this we follow the official guideline for calling convention https://mlir.llvm.org/docs/ConversionToLLVMDialect/#calling-convention.
emitCWrappers enables the following steps:

  1. Declares a new function _mlir_ciface_vulkanLaunch where memref arguments are converted to pointer-to-struct and the remaining arguments are converted as usual.
  2. Adds a body to the original function (making it non-external) that
    1. allocates a memref descriptor,
    2. populates it, and
    3. passes the pointer to it into the newly declared interface function
    4. collects the result of the call and returns it to the caller.
rriddle added inline comments.Mar 12 2020, 9:56 AM
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp
205

nit: You shouldn't need to construct a vector here, just use the operand_range directly. You can then change the llvm::drop_begin to operands.drop_front(...).

209

Why not use llvm::enumerate here? That would remove the need for the extra index variable.

denis13 updated this revision to Diff 249968.Mar 12 2020, 10:08 AM

Fix LINT issues.

denis13 marked 2 inline comments as done.Mar 12 2020, 10:21 AM
denis13 added inline comments.
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp
205

Thanks for point on this!

209

Thanks!

denis13 updated this revision to Diff 249976.Mar 12 2020, 10:23 AM

Address review comments.

Awesome! Thanks for the fix, Denis! Sorry for the delay. Overall LGTM; just a few nits. :)

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

Nit: isCInterfaceVulkanLaunchCallOp? Just spell it out to make it clear. Applies to other parameter names, etc.

149

I think we can use gpu::LaunchOp::kNumConfigOperands instead of having our own here.

157

Nit: SPIR-V

antiagainst requested changes to this revision.Mar 16 2020, 6:27 PM
This revision now requires changes to proceed.Mar 16 2020, 6:27 PM
denis13 updated this revision to Diff 250712.Mar 17 2020, 3:13 AM

Address review comments.

denis13 marked 3 inline comments as done and an inline comment as not done.Mar 17 2020, 3:17 AM
denis13 added inline comments.
mlir/lib/Conversion/GPUToVulkan/ConvertLaunchFuncToVulkanCalls.cpp
118

Thanks, fixed!

149

Thanks for pointing on this! Fixed.

157

Thanks!

antiagainst accepted this revision.Mar 17 2020, 4:44 AM
This revision is now accepted and ready to land.Mar 17 2020, 4:44 AM
This revision was automatically updated to reflect the committed changes.

@denis13 FYI if you haven't already, you really can ask for commit access: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

@mehdi_amini thanks, recently obtained it.