This is an archive of the discontinued LLVM Phabricator instance.

[mlir][GPU] Allow bare pointer memrefs when calling GPU kernels
ClosedPublic

Authored by krzysz00 on Jul 28 2022, 9:50 AM.

Details

Summary

In the ROCm runtime (and probably CUDA as well), all kernel arguments
are aligned. Therefore, enable using bare pointers for memref
arguments to kernels when these memrefs have static shape and a
trivial layout.

This is a substantial optimization to launching kernels that use
memrefs with known, static sizes, since it causes the kernel launch
packet to no longer include information already known to the kernel,
which can enable packing the kernel launch arguments into launch
packets instead of having to allocate an entire separate structure to
hold unneeded memref information.

Diff Detail

Event Timeline

krzysz00 created this revision.Jul 28 2022, 9:50 AM
krzysz00 requested review of this revision.Jul 28 2022, 9:50 AM
ftynse requested changes to this revision.Aug 2 2022, 2:58 AM
ftynse added inline comments.
mlir/lib/Conversion/GPUCommon/GPUOpsLowering.cpp
142
156–157

Shouldn't this be an assertion? We checked that the original type is a memref above, so it not being remapped is a converter misconfiguration.

mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp
659

This will discard any customizations that could have been applied to converter (e.g., support for additional types). I suppose we actually want a pseudo-copy constructor for LLVMTypeConverter that allows to take different options, or just a mechanism to change options with big warning flags that it should preserve consistency.

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp
44

Please document top-level entities.

49

Nit: getLayout().getAffineMap(), there is no guarantee the layout will be an` AffineMapAttr` in the longer run.

112

Nit: diagnostics should start with lower case.

This revision now requires changes to proceed.Aug 2 2022, 2:58 AM
krzysz00 updated this revision to Diff 449320.Aug 2 2022, 9:51 AM
krzysz00 marked 4 inline comments as done.

Adress review comments

krzysz00 marked an inline comment as done.Aug 2 2022, 9:53 AM

(Also , @ThomasRaoux - should this be added to the NVVM conversion? I'm not doing it right now because I can't test the change, but I thought I'd raise the possibility)

mlir/lib/Conversion/GPUCommon/GPUToLLVMConversion.cpp
659

Good call, changed

ftynse accepted this revision.Aug 2 2022, 11:25 AM

Thanks!

This revision is now accepted and ready to land.Aug 2 2022, 11:25 AM
This revision was automatically updated to reflect the committed changes.
krzysz00 marked an inline comment as done.