[mlir][DataLayout] Add a default memory space entry to the data layout. #127416
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-llvm Author: Fabian Mora (fabianmcg) ChangesThis patch adds methods to query the default memory space in the data layout. This is required as MLIR has no well defined default memory space unlike LLVM which has 0. While This patch also modifies the Patch is 27.82 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/127416.diff 13 Files Affected:
diff --git a/mlir/include/mlir/Dialect/DLTI/DLTIAttrs.td b/mlir/include/mlir/Dialect/DLTI/DLTIAttrs.td
index d54b3191eed7e..cd8bc5c196df1 100644
--- a/mlir/include/mlir/Dialect/DLTI/DLTIAttrs.td
+++ b/mlir/include/mlir/Dialect/DLTI/DLTIAttrs.td
@@ -74,6 +74,9 @@ def DLTI_DataLayoutSpecAttr :
/// Returns the endiannes identifier.
StringAttr getEndiannessIdentifier(MLIRContext *context) const;
+ /// Returns the default memory space identifier.
+ StringAttr getDefaultMemorySpaceIdentifier(MLIRContext *context) const;
+
/// Returns the alloca memory space identifier.
StringAttr getAllocaMemorySpaceIdentifier(MLIRContext *context) const;
diff --git a/mlir/include/mlir/Dialect/DLTI/DLTIBase.td b/mlir/include/mlir/Dialect/DLTI/DLTIBase.td
index f84149c43e0fc..7bdc25f38bdaf 100644
--- a/mlir/include/mlir/Dialect/DLTI/DLTIBase.td
+++ b/mlir/include/mlir/Dialect/DLTI/DLTIBase.td
@@ -49,6 +49,9 @@ def DLTI_Dialect : Dialect {
constexpr const static ::llvm::StringLiteral
kDataLayoutEndiannessLittle = "little";
+ constexpr const static ::llvm::StringLiteral
+ kDataLayoutDefaultMemorySpaceKey = "dlti.default_memory_space";
+
constexpr const static ::llvm::StringLiteral
kDataLayoutAllocaMemorySpaceKey = "dlti.alloca_memory_space";
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
index 7a7b659724f86..af88b377904c0 100644
--- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
+++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
@@ -74,6 +74,10 @@ getDefaultIndexBitwidth(Type type, const DataLayout &dataLayout,
/// DataLayoutInterface if specified, otherwise returns the default.
Attribute getDefaultEndianness(DataLayoutEntryInterface entry);
+/// Default handler for the default memory space request. Dispatches to the
+/// DataLayoutInterface if specified, otherwise returns the default.
+Attribute getDefaultMemorySpace(DataLayoutEntryInterface entry);
+
/// Default handler for alloca memory space request. Dispatches to the
/// DataLayoutInterface if specified, otherwise returns the default.
Attribute getDefaultAllocaMemorySpace(DataLayoutEntryInterface entry);
@@ -227,6 +231,9 @@ class DataLayout {
/// Returns the specified endianness.
Attribute getEndianness() const;
+ /// Returns the default memory space used for memory operations.
+ Attribute getDefaultMemorySpace() const;
+
/// Returns the memory space used for AllocaOps.
Attribute getAllocaMemorySpace() const;
@@ -276,7 +283,8 @@ class DataLayout {
/// Cache for the endianness.
mutable std::optional<Attribute> endianness;
- /// Cache for alloca, global, and program memory spaces.
+ /// Cache for default, alloca, global, and program memory spaces.
+ mutable std::optional<Attribute> defaultMemorySpace;
mutable std::optional<Attribute> allocaMemorySpace;
mutable std::optional<Attribute> programMemorySpace;
mutable std::optional<Attribute> globalMemorySpace;
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
index 0d09b92928fe3..db503daa6ed93 100644
--- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
+++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
@@ -135,6 +135,12 @@ def DataLayoutSpecInterface : AttrInterface<"DataLayoutSpecInterface", [DLTIQuer
/*methodName=*/"getEndiannessIdentifier",
/*args=*/(ins "::mlir::MLIRContext *":$context)
>,
+ InterfaceMethod<
+ /*description=*/"Returns the default memory space identifier.",
+ /*retTy=*/"::mlir::StringAttr",
+ /*methodName=*/"getDefaultMemorySpaceIdentifier",
+ /*args=*/(ins "::mlir::MLIRContext *":$context)
+ >,
InterfaceMethod<
/*description=*/"Returns the alloca memory space identifier.",
/*retTy=*/"::mlir::StringAttr",
@@ -469,6 +475,18 @@ def DataLayoutOpInterface : OpInterface<"DataLayoutOpInterface"> {
return ::mlir::detail::getDefaultEndianness(entry);
}]
>,
+ StaticInterfaceMethod<
+ /*description=*/"Returns the memory space used by the ABI computed "
+ "using the relevant entries. The data layout object "
+ "can be used for recursive queries.",
+ /*retTy=*/"::mlir::Attribute",
+ /*methodName=*/"getDefaultMemorySpace",
+ /*args=*/(ins "::mlir::DataLayoutEntryInterface":$entry),
+ /*methodBody=*/"",
+ /*defaultImplementation=*/[{
+ return ::mlir::detail::getDefaultMemorySpace(entry);
+ }]
+ >,
StaticInterfaceMethod<
/*description=*/"Returns the memory space used by the ABI computed "
"using the relevant entries. The data layout object "
@@ -615,11 +633,13 @@ def DataLayoutTypeInterface : TypeInterface<"DataLayoutTypeInterface"> {
InterfaceMethod<
/*desc=*/"Returns true if the two lists of entries are compatible, that "
"is, that `newLayout` spec entries can be nested in an op with "
- "`oldLayout` spec entries.",
+ "`oldLayout` spec entries. `newSpec` is provided to further"
+ "query data from the spec, e.g. the default address space.",
/*retTy=*/"bool",
/*methodName=*/"areCompatible",
/*args=*/(ins "::mlir::DataLayoutEntryListRef":$oldLayout,
- "::mlir::DataLayoutEntryListRef":$newLayout),
+ "::mlir::DataLayoutEntryListRef":$newLayout,
+ "::mlir::DataLayoutSpecInterface":$newSpec),
/*methodBody=*/"",
/*defaultImplementation=*/[{ return true; }]
>,
diff --git a/mlir/lib/Dialect/DLTI/DLTI.cpp b/mlir/lib/Dialect/DLTI/DLTI.cpp
index b057554c40d8c..0dd750a620b1d 100644
--- a/mlir/lib/Dialect/DLTI/DLTI.cpp
+++ b/mlir/lib/Dialect/DLTI/DLTI.cpp
@@ -318,7 +318,8 @@ combineOneSpec(DataLayoutSpecInterface spec,
"unexpected data layout entry for built-in type");
auto interface = cast<DataLayoutTypeInterface>(typeSample);
- if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second))
+ if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second,
+ spec))
return failure();
overwriteDuplicateEntries(entriesForType[kvp.first], kvp.second);
@@ -379,6 +380,12 @@ DataLayoutSpecAttr::getEndiannessIdentifier(MLIRContext *context) const {
return Builder(context).getStringAttr(DLTIDialect::kDataLayoutEndiannessKey);
}
+StringAttr DataLayoutSpecAttr::getDefaultMemorySpaceIdentifier(
+ MLIRContext *context) const {
+ return Builder(context).getStringAttr(
+ DLTIDialect::kDataLayoutDefaultMemorySpaceKey);
+}
+
StringAttr
DataLayoutSpecAttr::getAllocaMemorySpaceIdentifier(MLIRContext *context) const {
return Builder(context).getStringAttr(
@@ -602,7 +609,8 @@ class TargetDataLayoutInterface : public DataLayoutDialectInterface {
<< DLTIDialect::kDataLayoutEndiannessBig << "' or '"
<< DLTIDialect::kDataLayoutEndiannessLittle << "'";
}
- if (entryName == DLTIDialect::kDataLayoutAllocaMemorySpaceKey ||
+ if (entryName == DLTIDialect::kDataLayoutDefaultMemorySpaceKey ||
+ entryName == DLTIDialect::kDataLayoutAllocaMemorySpaceKey ||
entryName == DLTIDialect::kDataLayoutProgramMemorySpaceKey ||
entryName == DLTIDialect::kDataLayoutGlobalMemorySpaceKey ||
entryName == DLTIDialect::kDataLayoutStackAlignmentKey)
diff --git a/mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp b/mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
index 453b206de294e..aaf30df9482c9 100644
--- a/mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
+++ b/mlir/lib/Dialect/LLVMIR/IR/LLVMTypes.cpp
@@ -350,7 +350,8 @@ LLVMPointerType::getIndexBitwidth(const DataLayout &dataLayout,
}
bool LLVMPointerType::areCompatible(DataLayoutEntryListRef oldLayout,
- DataLayoutEntryListRef newLayout) const {
+ DataLayoutEntryListRef newLayout,
+ DataLayoutSpecInterface newSpec) const {
for (DataLayoutEntryInterface newEntry : newLayout) {
if (!newEntry.isTypeEntry())
continue;
@@ -598,7 +599,8 @@ static uint64_t extractStructSpecValue(Attribute attr, StructDLEntryPos pos) {
}
bool LLVMStructType::areCompatible(DataLayoutEntryListRef oldLayout,
- DataLayoutEntryListRef newLayout) const {
+ DataLayoutEntryListRef newLayout,
+ DataLayoutSpecInterface newSpec) const {
for (DataLayoutEntryInterface newEntry : newLayout) {
if (!newEntry.isTypeEntry())
continue;
diff --git a/mlir/lib/Dialect/Ptr/IR/PtrTypes.cpp b/mlir/lib/Dialect/Ptr/IR/PtrTypes.cpp
index 49c8ed977d50c..219a543acc32f 100644
--- a/mlir/lib/Dialect/Ptr/IR/PtrTypes.cpp
+++ b/mlir/lib/Dialect/Ptr/IR/PtrTypes.cpp
@@ -23,13 +23,12 @@ using namespace mlir::ptr;
constexpr const static unsigned kDefaultPointerSizeBits = 64;
constexpr const static unsigned kBitsInByte = 8;
-constexpr const static unsigned kDefaultPointerAlignment = 8;
-
-static Attribute getDefaultMemorySpace(PtrType ptr) { return nullptr; }
+constexpr const static unsigned kDefaultPointerAlignmentBits = 8;
/// Searches the data layout for the pointer spec, returns nullptr if it is not
/// found.
-static SpecAttr getPointerSpec(DataLayoutEntryListRef params, PtrType type) {
+static SpecAttr getPointerSpec(DataLayoutEntryListRef params, PtrType type,
+ Attribute defaultMemorySpace) {
for (DataLayoutEntryInterface entry : params) {
if (!entry.isTypeEntry())
continue;
@@ -41,20 +40,21 @@ static SpecAttr getPointerSpec(DataLayoutEntryListRef params, PtrType type) {
}
// If not found, and this is the pointer to the default memory space, assume
// 64-bit pointers.
- if (type.getMemorySpace() == getDefaultMemorySpace(type))
+ if (type.getMemorySpace() == defaultMemorySpace)
return SpecAttr::get(type.getContext(), kDefaultPointerSizeBits,
- kDefaultPointerAlignment, kDefaultPointerAlignment,
- kDefaultPointerSizeBits);
+ kDefaultPointerAlignmentBits,
+ kDefaultPointerAlignmentBits, kDefaultPointerSizeBits);
return nullptr;
}
bool PtrType::areCompatible(DataLayoutEntryListRef oldLayout,
- DataLayoutEntryListRef newLayout) const {
+ DataLayoutEntryListRef newLayout,
+ DataLayoutSpecInterface newSpec) const {
for (DataLayoutEntryInterface newEntry : newLayout) {
if (!newEntry.isTypeEntry())
continue;
uint32_t size = kDefaultPointerSizeBits;
- uint32_t abi = kDefaultPointerAlignment;
+ uint32_t abi = kDefaultPointerAlignmentBits;
auto newType = llvm::cast<PtrType>(llvm::cast<Type>(newEntry.getKey()));
const auto *it =
llvm::find_if(oldLayout, [&](DataLayoutEntryInterface entry) {
@@ -65,10 +65,13 @@ bool PtrType::areCompatible(DataLayoutEntryListRef oldLayout,
return false;
});
if (it == oldLayout.end()) {
+ Attribute defaultMemorySpace =
+ mlir::detail::getDefaultMemorySpace(newSpec.getSpecForIdentifier(
+ newSpec.getDefaultMemorySpaceIdentifier(getContext())));
it = llvm::find_if(oldLayout, [&](DataLayoutEntryInterface entry) {
if (auto type = llvm::dyn_cast_if_present<Type>(entry.getKey())) {
auto ptrTy = llvm::cast<PtrType>(type);
- return ptrTy.getMemorySpace() == getDefaultMemorySpace(ptrTy);
+ return ptrTy.getMemorySpace() == defaultMemorySpace;
}
return false;
});
@@ -90,43 +93,44 @@ bool PtrType::areCompatible(DataLayoutEntryListRef oldLayout,
uint64_t PtrType::getABIAlignment(const DataLayout &dataLayout,
DataLayoutEntryListRef params) const {
- if (SpecAttr spec = getPointerSpec(params, *this))
+ Attribute defaultMemorySpace = dataLayout.getDefaultMemorySpace();
+ if (SpecAttr spec = getPointerSpec(params, *this, defaultMemorySpace))
return spec.getAbi() / kBitsInByte;
- return dataLayout.getTypeABIAlignment(
- get(getContext(), getDefaultMemorySpace(*this)));
+ return dataLayout.getTypeABIAlignment(get(getContext(), defaultMemorySpace));
}
std::optional<uint64_t>
PtrType::getIndexBitwidth(const DataLayout &dataLayout,
DataLayoutEntryListRef params) const {
- if (SpecAttr spec = getPointerSpec(params, *this)) {
+ Attribute defaultMemorySpace = dataLayout.getDefaultMemorySpace();
+ if (SpecAttr spec = getPointerSpec(params, *this, defaultMemorySpace)) {
return spec.getIndex() == SpecAttr::kOptionalSpecValue ? spec.getSize()
: spec.getIndex();
}
- return dataLayout.getTypeIndexBitwidth(
- get(getContext(), getDefaultMemorySpace(*this)));
+ return dataLayout.getTypeIndexBitwidth(get(getContext(), defaultMemorySpace));
}
llvm::TypeSize PtrType::getTypeSizeInBits(const DataLayout &dataLayout,
DataLayoutEntryListRef params) const {
- if (SpecAttr spec = getPointerSpec(params, *this))
+ Attribute defaultMemorySpace = dataLayout.getDefaultMemorySpace();
+ if (SpecAttr spec = getPointerSpec(params, *this, defaultMemorySpace))
return llvm::TypeSize::getFixed(spec.getSize());
// For other memory spaces, use the size of the pointer to the default memory
// space.
- return dataLayout.getTypeSizeInBits(
- get(getContext(), getDefaultMemorySpace(*this)));
+ return dataLayout.getTypeSizeInBits(get(getContext(), defaultMemorySpace));
}
uint64_t PtrType::getPreferredAlignment(const DataLayout &dataLayout,
DataLayoutEntryListRef params) const {
- if (SpecAttr spec = getPointerSpec(params, *this))
+ Attribute defaultMemorySpace = dataLayout.getDefaultMemorySpace();
+ if (SpecAttr spec = getPointerSpec(params, *this, defaultMemorySpace))
return spec.getPreferred() / kBitsInByte;
return dataLayout.getTypePreferredAlignment(
- get(getContext(), getDefaultMemorySpace(*this)));
+ get(getContext(), defaultMemorySpace));
}
LogicalResult PtrType::verifyEntries(DataLayoutEntryListRef entries,
diff --git a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
index 049d7f123cec8..4d2f992340279 100644
--- a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
+++ b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
@@ -246,6 +246,17 @@ Attribute mlir::detail::getDefaultEndianness(DataLayoutEntryInterface entry) {
return entry.getValue();
}
+// Returns the default memory space if specified in the given entry. If the
+// entry is empty the default memory space represented by an empty attribute is
+// returned.
+Attribute mlir::detail::getDefaultMemorySpace(DataLayoutEntryInterface entry) {
+ if (entry == DataLayoutEntryInterface()) {
+ return Attribute();
+ }
+
+ return entry.getValue();
+}
+
// Returns the memory space used for alloca operations if specified in the
// given entry. If the entry is empty the default memory space represented by
// an empty attribute is returned.
@@ -596,6 +607,22 @@ mlir::Attribute mlir::DataLayout::getEndianness() const {
return *endianness;
}
+mlir::Attribute mlir::DataLayout::getDefaultMemorySpace() const {
+ checkValid();
+ if (defaultMemorySpace)
+ return *defaultMemorySpace;
+ DataLayoutEntryInterface entry;
+ if (originalLayout)
+ entry = originalLayout.getSpecForIdentifier(
+ originalLayout.getDefaultMemorySpaceIdentifier(
+ originalLayout.getContext()));
+ if (auto iface = dyn_cast_or_null<DataLayoutOpInterface>(scope))
+ defaultMemorySpace = iface.getDefaultMemorySpace(entry);
+ else
+ defaultMemorySpace = detail::getDefaultMemorySpace(entry);
+ return *defaultMemorySpace;
+}
+
mlir::Attribute mlir::DataLayout::getAllocaMemorySpace() const {
checkValid();
if (allocaMemorySpace)
diff --git a/mlir/test/Dialect/LLVMIR/layout.mlir b/mlir/test/Dialect/LLVMIR/layout.mlir
index 4813089282fbc..7aa8cfcdaa46b 100644
--- a/mlir/test/Dialect/LLVMIR/layout.mlir
+++ b/mlir/test/Dialect/LLVMIR/layout.mlir
@@ -6,6 +6,7 @@ module {
// CHECK: alignment = 8
// CHECK: alloca_memory_space = 0
// CHECK: bitsize = 64
+ // CHECK: default_memory_space = 0
// CHECK: endianness = ""
// CHECK: global_memory_space = 0
// CHECK: index = 64
@@ -17,6 +18,7 @@ module {
// CHECK: alignment = 8
// CHECK: alloca_memory_space = 0
// CHECK: bitsize = 64
+ // CHECK: default_memory_space = 0
// CHECK: endianness = ""
// CHECK: global_memory_space = 0
// CHECK: index = 64
@@ -28,6 +30,7 @@ module {
// CHECK: alignment = 8
// CHECK: alloca_memory_space = 0
// CHECK: bitsize = 64
+ // CHECK: default_memory_space = 0
// CHECK: endianness = ""
// CHECK: global_memory_space = 0
// CHECK: index = 64
@@ -47,6 +50,7 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<
#dlti.dl_entry<!llvm.ptr<5>, dense<[64, 64, 64]> : vector<3xi64>>,
#dlti.dl_entry<!llvm.ptr<4>, dense<[32, 64, 64, 24]> : vector<4xi64>>,
#dlti.dl_entry<"dlti.endianness", "little">,
+ #dlti.dl_entry<"dlti.default_memory_space", 7 : ui64>,
#dlti.dl_entry<"dlti.alloca_memory_space", 5 : ui64>,
#dlti.dl_entry<"dlti.global_memory_space", 2 : ui64>,
#dlti.dl_entry<"dlti.program_memory_space", 3 : ui64>,
@@ -57,6 +61,7 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<
// CHECK: alignment = 4
// CHECK: alloca_memory_space = 5
// CHECK: bitsize = 32
+ // CHECK: default_memory_space = 7
// CHECK: endianness = "little"
// CHECK: global_memory_space = 2
// CHECK: index = 32
@@ -68,6 +73,7 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<
// CHECK: alignment = 4
// CHECK: alloca_memory_space = 5
// CHECK: bitsize = 32
+ // CHECK: default_memory_space = 7
// CHECK: endianness = "little"
// CHECK: global_memory_space = 2
// CHECK: index = 32
@@ -79,6 +85,7 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<
// CHECK: alignment = 8
// CHECK: alloca_memory_space = 5
// CHECK: bitsize = 64
+ // CHECK: default_memory_space = 7
// CHECK: endianness = "little"
// CHECK: global_memory_space = 2
// CHECK: index = 64
@@ -90,6 +97,7 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<
// CHECK: alignment = 8
// CHECK: alloca_memory_space = 5
// CHECK: bitsize = 32
+ // CHECK: default_memory_space = 7
// CHECK: endianness = "little"
// CHECK: global_memory_space = 2
// CHECK: index = 24
diff --git a/mlir/test/Dialect/Ptr/layout.mlir b/mlir/test/Dialect/Ptr/layout.mlir
index 73189a388942a..356c57402651f 100644
--- a/mlir/test/Dialect/Ptr/layout.mlir
+++ b/mlir/test/Dialect/Ptr/layout.mlir
@@ -4,16 +4,18 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<
#dlti.dl_entry<!ptr.ptr, #ptr.spec<size = 32, abi = 32, preferred = 64>>,
#dlti.dl_entry<!ptr.ptr<5>,#ptr.spec<size = 64, abi = 64, preferred = 64>>,
#dlti.dl_entry<!ptr.ptr<4>, #ptr.spec<size = 32, abi = 64, preferred = 64, index = 24>>,
+ #dlti.dl_entry<"dlti.default_memory_space", 7 : ui64>,
#dlti.dl_entry<"dlti.alloca_memory_space", 5 : ui64>,
#dlti.dl_entry<"dlti.global_memory_space", 2 : ui64>,
#dlti.dl_entry<"dlti.program_memory_space", 3 : ui64>,
#dlti.dl_entry<"dlti.stack_alignment", 128 : i64>
>} {
- // CHECK: @spec
+ // CHECK-LABEL: @spec
func.func @spec() {
// CHECK: alignment = 4
// CHECK: alloca_memory_space = 5
// CHECK: bitsize = 32
+ // CHECK: default_memory_space = 7
// CHECK: global_memory_space = 2
// CHECK: index = 32
// CHECK: preferred = 8
@@ -21,19 +23,21 @@ module attributes { dlti.dl_spec = #dlti.dl_spec<
// CHECK: size = 4
// CHECK: stack_alignment = 128
"test.data_layout_query"() : () -> !ptr.ptr
- // CHECK: alignment = 4
+ // CHECK: alignment = 1
// CHECK: alloca_memory_space = 5
- // CHECK: bitsize = 32
+ // CHECK: bitsize = 64
+ // CHECK: default_memory_space ...
[truncated]
|
joker-eph
reviewed
Feb 17, 2025
joker-eph
reviewed
Feb 17, 2025
|
Ping for review. |
rolfmorel
reviewed
Feb 25, 2025
mlir/lib/Dialect/DLTI/DLTI.cpp
Outdated
| @@ -318,7 +318,8 @@ combineOneSpec(DataLayoutSpecInterface spec, | |||
| "unexpected data layout entry for built-in type"); | |||
|
|
|||
| auto interface = cast<DataLayoutTypeInterface>(typeSample); | |||
| if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second)) | |||
| if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second, | |||
There was a problem hiding this comment.
Could you please explain why passing the whole of spec here is necessary?
For example, why couldn't "dlti.default_memory_space" just be another "newLayout" entry that gets appropriately checked against "oldLayout" entries (however areCompatible's implementers define that)?
I am guessing part of the issue is that this concerns a per type compatibility check whereas you introduced another global DLTI key. I see the tension there, though feel passing the whole of spec is probably too big a hammer to apply here.
There was a problem hiding this comment.
I am guessing part of the issue is that this concerns a per type compatibility check whereas you introduced another global DLTI key.
That's exactly the reason. Further, passing just the value for the default_memory_space was too specific.
My reasoning is: to determine DL type compatibility the method should have a view of all the global entries. Eventually it would be nice to be able to collect all the global entries with a method and pass that, but that's out of scope of this patch.
There was a problem hiding this comment.
I see, though am not able to say if DL checks for types should really have access to the whole spec.
Also is spec here the right spec to pass? The specs are in the process of being combined at this point, right? spec is just the latest to be added, I think (it's a while since I looked at that code).
@Dinistro, do you happen to have any comments on this approach and the proposed resolution?
There was a problem hiding this comment.
Sorry for the late reply.
I guess the pointer type is somewhat special in that regard as this information is only accessible when going to the spec. Considering this, and that there might be other obscure use cases for this, I think this is probably acceptable.
There was a problem hiding this comment.
As I don't have any better suggestions at the moment (though I also haven't been able to think about it much), I am happy to go with @Dinistro 's "this is probably acceptable."
Maybe someone finds a better way later on.
There was a problem hiding this comment.
That works for your usage: the
get...Identifiermethods are okay to access this way butnewSpecitself is just the latest individual spec to get merged into the lists andareCompatiblewill be called with each of thesespecs in turn (in addition to theentriesForIDlist which is the correct merging of all hitherto specs). Also, one of thesespecs will define thenewLayout. Personally, I am getting a waft of code smells.
The issue here is that there's no way to identify which entry is defining the default global memory space without knowing the key.
Let's assume we have something like:
DL1 /*interface methods*/ [default_identifier = "dlti.default_memory_space"] {
DL2 /*interface methods*/ [default_identifier = "my_dlti.default_memory_space"] /*data layout entries*/ <ptr spec> {
# ptr usage
}
}
Then DL2 (newSpec) should query with my_dlti.default_memory_space and not dlti.default_memory_space. Which, as you say, it smells and that's why I say we need to revisit identified entries.
@rolfmorel since I don't think this is a simple TODO, because the issue is not that simple and extends beyond this patch, how about we create an gh issue to track that instead of a TODO?
There was a problem hiding this comment.
Great example of why these key identifiers should probably just be static. Do they even combine if the default identifiers don't match? And how does it work in the following case:
DL1 /*interface methods*/ [default_identifier = "dlti.default_memory_space"] {
DL2 /*interface methods*/ [default_identifier = "my_dlti.default_memory_space"] /*data layout entries*/ {
DL3 /*interface methods*/ [default_identifier = "dlti.default_memory_space"] <ptr spec> {
# ptr usage
}
}
}
Does your code do the right thing when curSpec becomes DL3 in the last combineOneSpec call?
Point taken about this being a complex issue. How about you create a Github issue and link to it from a TODO in this PR. I am afraid otherwise GH issues are way too easy to lose track off.
There was a problem hiding this comment.
@rolfmorel I created the issue and added the comment. Let's follow up the discussion on the issue. I agree that the simplest solution is just making those values constant.
There was a problem hiding this comment.
@rolfmorel is there any other comment on this PR left?
There was a problem hiding this comment.
Let's leave this comment chain "unresolved" shall we? It is easier to find that way for people coming from #130321 .
Dinistro
reviewed
Feb 27, 2025
mlir/lib/Dialect/DLTI/DLTI.cpp
Outdated
| @@ -318,7 +318,8 @@ combineOneSpec(DataLayoutSpecInterface spec, | |||
| "unexpected data layout entry for built-in type"); | |||
|
|
|||
| auto interface = cast<DataLayoutTypeInterface>(typeSample); | |||
| if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second)) | |||
| if (!interface.areCompatible(entriesForType.lookup(kvp.first), kvp.second, | |||
There was a problem hiding this comment.
Sorry for the late reply.
I guess the pointer type is somewhat special in that regard as this information is only accessible when going to the spec. Considering this, and that there might be other obscure use cases for this, I think this is probably acceptable.
This patch adds methods to query the default memory space in the data layout. This is required as MLIR has no well defined default memory space unlike LLVM which has 0. While `nullptr` is a candidate for default memory space, the `ptr` dialect will remove the possibility for `nullptr` memory spaces to avoid undefined semantics. This patch also modifies the `DataLayoutTypeInterface::areCompatible` to include the new DL spec, as it is needed to query the default memory space.
Co-authored-by: Christian Ulmann <[email protected]>
fabianmcg
force-pushed
the
pr-default-space
branch
from
1cc473b to
c3224ad
Compare
rolfmorel
reviewed
Mar 10, 2025
rolfmorel
reviewed
Mar 10, 2025
rolfmorel
reviewed
Mar 10, 2025
rolfmorel
approved these changes
Mar 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds a default memory space attribute to the DL and adds methods to query the attribute. This is required as MLIR has no well defined default memory space unlike LLVM which has 0. While
nullptris a candidate for default memory space, theptrdialect will remove the possibility fornullptrmemory spaces to avoid undefined semantics.This patch also modifies the
DataLayoutTypeInterface::areCompatibleto include the new DL spec, as it is needed to query the default memory space.