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

[Lei Zhang via Phabricator via llvm-commits]

antiagainst added inline comments.


================
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())
----------------
mravishankar wrote:
> Nit : Update the error message
I plan to turn this into a proper SPIR-V dialect attribute too. It's becoming more annoying seeing the complicated internal structure of a dictionary attribute and we'll have more fields later.


================
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)                                \
----------------
mravishankar wrote:
> 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. 
Adjusted. I guess the benefit of having similar mapping is mostly cognitive so that folks familiar with the NVVM mapping is not that surprised. But I'd avoid having non-1-1 mapping given it's not round trippable and also confusing. So for now, 

Generic: 1
StorageBuffer: 0
Workgroup: 3
Uniform: 4
Private: 5
Function: 6
PushConstant: 7

We can swap Generic and StorageBuffer later after IREE side is fixed.


================
Comment at: mlir/lib/Dialect/SPIRV/TargetAndABI.cpp:15
 #include "mlir/IR/SymbolTable.h"
+#include "mlir/Support/LLVM.h"
 
----------------
rriddle wrote:
> Is this not included transitively already?
Yes. I think it was added by YouCompleteMe. Updated and tweaked some configuration for my vim plugins, now seeing this. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76241