[PATCH] D76241: [mlir][spirv] Use memref memory space for storage class
Lei Zhang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 17 07:25:12 PDT 2020
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
More information about the llvm-commits
mailing list