[PATCH] D76241: [mlir][spirv] Use memref memory space for storage class

Mon Mar 16 12:03:25 PDT 2020

mravishankar requested changes to this revision.
mravishankar added inline comments.
This revision now requires changes to proceed.

================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVDialect.cpp:887
               "32-bit integer attributes: 'descriptor_set', 'binding', and "
               "'storage_class'";
+  if (varABIAttr.storage_class() && !valueType.isIntOrIndexOrFloat())
----------------
Nit : Update the error message

================
Comment at: mlir/lib/Dialect/SPIRV/SPIRVLowering.cpp:48
+// eventually after memref supports it.
+#define STORAGE_SPACE_MAP_LIST(MAP_FN)                                         \
+  MAP_FN(spirv::StorageClass::StorageBuffer, 0)                                \
----------------
we should try to keep it close to what GPU dialect uses. There it is using a convention that NVVM adopts (https://docs.nvidia.com/cuda/nvvm-ir-spec/index.html#address-spaces).

I think this mapping would be most consistent (there is no direct mapping)

StorageBuffer : 0, 1
Workgroup : 3
PushConstant : 4
Private : 5.

Lets leave 2 out since it is reserved. 

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76241/new/

https://reviews.llvm.org/D76241