This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Add a pass to map memref memory space
ClosedPublic

Authored by antiagainst on Jul 21 2022, 5:26 PM.

Details

Summary

MemRef types now can carry an attribute to represent the memory
space. Still, upper layers in the compilation stack mostly use
nuemric values. They don't mean much (other than differentiating
separate memory domains) in MLIR's multi-level settings. Those
numeric memory space inside MemRef types need to be translated
into concrete SPIR-V storage classes during lowering to pin down
to concrete memory types.

Thus far we have been hardcoding an arbitrary mapping from memory
space to storage class for converting MemRef types. This works fine
for only targeting Vulkan; it falls apart if we want to target other
SPIR-V consumers like OpenCL, as different consumers might want
different storage classes for the buffer/variable of the same
lifetime. For example, StorageClass in Vulkan vs. CrossWorkgroup
in OpenCL.

So putting up a new pass to let the user to control how to map
MemRef memory spaces into SPIR-V storage classes. This provides
more flexibility and can address the awkwardness in the current
SPIR-V type converter. This pass should be the prelimiary step
towards lowering MemRef related types/ops into SPIR-V.

Diff Detail

Event Timeline

antiagainst created this revision.Jul 21 2022, 5:26 PM
antiagainst requested review of this revision.Jul 21 2022, 5:26 PM
antiagainst retitled this revision from [mlir][spirv] Add a passe to map memref memory space to [mlir][spirv] Add a pass to map memref memory space.Jul 21 2022, 5:34 PM
mravishankar added inline comments.Jul 21 2022, 10:43 PM
mlir/include/mlir/Conversion/MemRefToSPIRV/MemRefToSPIRVPass.h
31

I dont have a strong opinion here, but IIUC using this kind of signature for creating passes can make it hard for reproducers to work.. From past experience having a pass for this is not really useful. It is better to just have a utility function that the client can wrap within a pass. This works for reproducers.

Address comments

Remove unnecessary entry points

antiagainst marked an inline comment as done.
antiagainst added inline comments.
mlir/include/mlir/Conversion/MemRefToSPIRV/MemRefToSPIRVPass.h
31

Good point. I've changed to expose the various bits so they can be put into a downstream pass.

kuhar added a comment.Aug 4 2022, 10:35 AM

Just some nits

mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
101

nit 1: consider defining these two on two separate lines. I think this is not covered by the llvm coding standards, so it's up to you.
nit 2: could you add a comment explaining how the small size was picked? I think it has to do with the number of SPIR-V storage classes, doesn't it? Or consider using the default size if there is no clear limit.

103–105

Alternatively, I think you can do something like this:

auto inputs = to_vector(map_range(type.getInputs(), convertType));
mlir/test/Conversion/MemRefToSPIRV/map-storage-class.mlir
2

Would it make sense to add another test with an ill-formed mapping lists?

mravishankar accepted this revision.Aug 4 2022, 7:52 PM
This revision is now accepted and ready to land.Aug 4 2022, 7:52 PM
antiagainst marked 4 inline comments as done.Aug 5 2022, 9:20 AM
antiagainst added inline comments.
mlir/lib/Conversion/MemRefToSPIRV/MapMemRefStorageClassPass.cpp
101

It's mostly from experience of how many inputs/results a function can have. Dropped the explicit number there.

103–105

convertType is a class method here. I think it's fine to keep the existing way to make it more readable.

mlir/test/Conversion/MemRefToSPIRV/map-storage-class.mlir
2

This will be changed to read clinent-api later. So I won't bother it for now.

This revision was landed with ongoing or failed builds.Aug 5 2022, 9:23 AM
This revision was automatically updated to reflect the committed changes.
antiagainst marked 3 inline comments as done.