This patch adds alloca address space information to the data layout interface
and implementation in the DLTI dialect. This is needed for targets that use
separate address spaces for local/stack data.
Details
Diff Detail
Event Timeline
Since we want to be able to use this whenever someone's creating a memref::AllocaOp, we want a generic Attribute-valued memory space key.
However, we could also have both the unsigned-valued allocaAddressSpace and the Attribute-valued allocaMemorySpace as separate keys, with addressSpace used in LLVM lowering on down and allocaMemorySpace used in MLIR proper. Though that'd require some documentation to make sure people understand which is which ...
mlir/include/mlir/Dialect/DLTI/DLTI.h | ||
---|---|---|
102 |
| |
mlir/include/mlir/Interfaces/DataLayoutInterfaces.h | ||
61 | I'd argue this should also be Attribute, and that, when dealing with the *ToLLVMs, it should be sent through the attribute conversion process. | |
192 | mutable Attribute memorySpace | |
mlir/lib/Dialect/DLTI/DLTI.cpp | ||
340 | Strong disagree with the restriction to integer attributes, consider that this needs to work for #gpu.address_space<Private> |
... in further comments, if we want to go with the address space and memory space as distinct data layout entries system, I'd be willing to review this patch on its own and leave the memory space part to another patch.
It would probably be better to have a generic entry system that can work for anything
mlir/include/mlir/Interfaces/DataLayoutInterfaces.h | ||
---|---|---|
61 | Yes, that is more general. I will work on an updated patch. | |
192 | Yes this makes sense. Each dialect (like LLVMIR) that uses the data layout could have their own convenience functions to handle the values. Where would the best way to put these functions? |
Re the default value of getAllocaMemorySpcae(), that would be the null attribute, aka {} aka nullptr, which is the current default.
Use Attribute instead of integers to encode memory spaces. Change address space -> memory space.
Looks good to me - though I'm not sure if we want @ftynse or someone else more familiar with DTLI to take a look before landing.