[Mlir-commits] [mlir] DLTI: Restricting value type to StringAttr in a target device spec (PR #96706)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Tue Jun 25 15:12:10 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir
Author: Niranjan Hasabnis (nhasabni)
<details>
<summary>Changes</summary>
So instead of
```
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
#dlti.dl_entry<"max_vector_op_width", 64>>,
"GPU": #dlti.target_device_spec<
#dlti.dl_entry<"max_vector_op_width", 128>>
>} {}
```
this PR supports following:
```
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
#dlti.dl_entry<"max_vector_op_width", "64">>,
"GPU": #dlti.target_device_spec<
#dlti.dl_entry<"max_vector_op_width", "128">>
>} {}
```
---
Full diff: https://github.com/llvm/llvm-project/pull/96706.diff
8 Files Affected:
- (modified) mlir/include/mlir/Interfaces/DataLayoutInterfaces.h (+5-5)
- (modified) mlir/include/mlir/Interfaces/DataLayoutInterfaces.td (+3-3)
- (modified) mlir/lib/Dialect/DLTI/DLTI.cpp (+6)
- (modified) mlir/lib/Interfaces/DataLayoutInterfaces.cpp (+6-7)
- (modified) mlir/test/Dialect/DLTI/invalid.mlir (+19-6)
- (modified) mlir/test/Dialect/DLTI/roundtrip.mlir (+4-4)
- (modified) mlir/test/Dialect/DLTI/valid.mlir (+24-24)
- (modified) mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp (+10-10)
``````````diff
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
index 4cbad38df42ba..ec90c82131246 100644
--- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
+++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
@@ -93,8 +93,8 @@ uint64_t getDefaultStackAlignment(DataLayoutEntryInterface entry);
/// Returns the value of the property from the specified DataLayoutEntry. If the
/// property is missing from the entry, returns std::nullopt.
-std::optional<int64_t>
-getDevicePropertyValueAsInt(DataLayoutEntryInterface entry);
+std::optional<StringAttr>
+getDevicePropertyValue(DataLayoutEntryInterface entry);
/// Given a list of data layout entries, returns a new list containing the
/// entries with keys having the given type ID, i.e. belonging to the same type
@@ -246,9 +246,9 @@ class DataLayout {
/// Returns the value of the specified property if the property is defined for
/// the given device ID, otherwise returns std::nullopt.
- std::optional<int64_t>
- getDevicePropertyValueAsInt(TargetSystemSpecInterface::DeviceID,
- StringAttr propertyName) const;
+ std::optional<StringAttr>
+ getDevicePropertyValue(TargetSystemSpecInterface::DeviceID,
+ StringAttr propertyName) const;
private:
/// Combined layout spec at the given scope.
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
index 8ff0e3f5f863b..4975e36551a42 100644
--- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
+++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
@@ -468,12 +468,12 @@ def DataLayoutOpInterface : OpInterface<"DataLayoutOpInterface"> {
StaticInterfaceMethod<
/*description=*/"Returns the value of the property, if the property is "
"defined. Otherwise, it returns std::nullopt.",
- /*retTy=*/"std::optional<int64_t>",
- /*methodName=*/"getDevicePropertyValueAsInt",
+ /*retTy=*/"std::optional<StringAttr>",
+ /*methodName=*/"getDevicePropertyValue",
/*args=*/(ins "::mlir::DataLayoutEntryInterface":$entry),
/*methodBody=*/"",
/*defaultImplementation=*/[{
- return ::mlir::detail::getDevicePropertyValueAsInt(entry);
+ return ::mlir::detail::getDevicePropertyValue(entry);
}]
>
];
diff --git a/mlir/lib/Dialect/DLTI/DLTI.cpp b/mlir/lib/Dialect/DLTI/DLTI.cpp
index 592fced0b2abd..080cba397b549 100644
--- a/mlir/lib/Dialect/DLTI/DLTI.cpp
+++ b/mlir/lib/Dialect/DLTI/DLTI.cpp
@@ -352,9 +352,15 @@ TargetDeviceSpecAttr::verify(function_ref<InFlightDiagnostic()> emitError,
<< "dlti.target_device_spec does not allow type as a key: "
<< type;
} else {
+ // Check that keys in a target device spec are unique.
auto id = entry.getKey().get<StringAttr>();
if (!ids.insert(id).second)
return emitError() << "repeated layout entry key: " << id.getValue();
+
+ // Check that values in a target device spec are of StringAttr type.
+ if (!llvm::isa<StringAttr>(entry.getValue()))
+ return emitError() << "dlti.target_device_spec supports values of "
+ "StringAttr type only.";
}
}
diff --git a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
index df86bd757b628..b8bbe27fa15dd 100644
--- a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
+++ b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
@@ -293,13 +293,12 @@ mlir::detail::getDefaultStackAlignment(DataLayoutEntryInterface entry) {
return value.getValue().getZExtValue();
}
-std::optional<int64_t>
-mlir::detail::getDevicePropertyValueAsInt(DataLayoutEntryInterface entry) {
+std::optional<StringAttr>
+mlir::detail::getDevicePropertyValue(DataLayoutEntryInterface entry) {
if (entry == DataLayoutEntryInterface())
return std::nullopt;
- auto value = cast<IntegerAttr>(entry.getValue());
- return value.getValue().getZExtValue();
+ return cast<StringAttr>(entry.getValue());
}
DataLayoutEntryList
@@ -661,7 +660,7 @@ uint64_t mlir::DataLayout::getStackAlignment() const {
return *stackAlignment;
}
-std::optional<int64_t> mlir::DataLayout::getDevicePropertyValueAsInt(
+std::optional<StringAttr> mlir::DataLayout::getDevicePropertyValue(
TargetSystemSpecInterface::DeviceID deviceID,
StringAttr propertyName) const {
checkValid();
@@ -676,9 +675,9 @@ std::optional<int64_t> mlir::DataLayout::getDevicePropertyValueAsInt(
// missing, we return std::nullopt so that the users can resort to
// the default value however they want.
if (auto iface = dyn_cast_or_null<DataLayoutOpInterface>(scope))
- return iface.getDevicePropertyValueAsInt(entry);
+ return iface.getDevicePropertyValue(entry);
else
- return detail::getDevicePropertyValueAsInt(entry);
+ return detail::getDevicePropertyValue(entry);
}
//===----------------------------------------------------------------------===//
diff --git a/mlir/test/Dialect/DLTI/invalid.mlir b/mlir/test/Dialect/DLTI/invalid.mlir
index d732fb462e68e..5eb9559e5d415 100644
--- a/mlir/test/Dialect/DLTI/invalid.mlir
+++ b/mlir/test/Dialect/DLTI/invalid.mlir
@@ -113,7 +113,7 @@ module attributes {
// expected-error at +2 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
dlti.target_system_spec = #dlti.target_system_spec<
: #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>
>} {}
// -----
@@ -126,7 +126,7 @@ module attributes {
// expected-error at +2 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
dlti.target_system_spec = #dlti.target_system_spec<
0: #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096: i32>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>
>} {}
// -----
@@ -137,9 +137,9 @@ module attributes {
// expected-error at below {{repeated Device ID in dlti.target_system_spec: "CPU"}}
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096>>,
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>,
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 8192>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">>
>} {}
// -----
@@ -152,6 +152,19 @@ module attributes {
// expected-error at +5 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096>,
- #dlti.dl_entry<"L1_cache_size_in_bytes", 8192>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">,
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">>
+ >} {}
+
+// -----
+
+module attributes {
+ // Repeated DLTI entry
+ //
+ // expected-error at +4 {{dlti.target_device_spec supports values of StringAttr type only.}}
+ // expected-error at +5 {{Error in parsing target device spec}}
+ // expected-error at +4 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef<DeviceIDTargetDeviceSpecPair>`}}
+ dlti.target_system_spec = #dlti.target_system_spec<
+ "CPU": #dlti.target_device_spec<
+ #dlti.dl_entry<"L1_cache_size_in_bytes", 4096>>
>} {}
diff --git a/mlir/test/Dialect/DLTI/roundtrip.mlir b/mlir/test/Dialect/DLTI/roundtrip.mlir
index 7b8255ad71d4d..c5be3bc805330 100644
--- a/mlir/test/Dialect/DLTI/roundtrip.mlir
+++ b/mlir/test/Dialect/DLTI/roundtrip.mlir
@@ -58,16 +58,16 @@
// CHECK: module attributes {
// CHECK: dlti.target_system_spec = #dlti.target_system_spec<
// CHECK: "CPU" : #dlti.target_device_spec<
-// CHECK: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>,
+// CHECK: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", "4096">>,
// CHECK: "GPU" : #dlti.target_device_spec<
-// CHECK: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>>
+// CHECK: #dlti.dl_entry<"dlti.max_vector_op_width", "128">>
// CHECK: >} {
// CHECK: }
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096: ui32>>,
+ #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", "4096">>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"dlti.max_vector_op_width", 128: ui32>>
+ #dlti.dl_entry<"dlti.max_vector_op_width", "128">>
>} {}
diff --git a/mlir/test/Dialect/DLTI/valid.mlir b/mlir/test/Dialect/DLTI/valid.mlir
index 98e9d0b8de491..2084168d865df 100644
--- a/mlir/test/Dialect/DLTI/valid.mlir
+++ b/mlir/test/Dialect/DLTI/valid.mlir
@@ -5,17 +5,17 @@
// CHECK: module attributes {
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>>,
+// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>,
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i32>>
+// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">>
// CHECK-SAME: >} {
// CHECK: }
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096: i32>>,
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128: i32>>
+ #dlti.dl_entry<"max_vector_op_width", "128">>
>} {}
// -----
@@ -23,17 +23,17 @@ module attributes {
// CHECK: module attributes {
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>>,
+// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>,
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i32>>
+// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">>
// CHECK-SAME: >} {
// CHECK: }
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096: i32>>,
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 8192: i32>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">>
>} {}
// -----
@@ -41,17 +41,17 @@ module attributes {
// CHECK: module attributes {
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i64>>,
+// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>,
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i64>>
+// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">>
// CHECK-SAME: >} {
// CHECK: }
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096: i64>>,
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 8192: i64>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">>
>} {}
// -----
@@ -59,17 +59,17 @@ module attributes {
// CHECK: module attributes {
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 64 : i32>>,
+// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "64">>,
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i32>>
+// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">>
// CHECK-SAME: >} {
// CHECK: }
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 64: i32>>,
+ #dlti.dl_entry<"max_vector_op_width", "64">>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128: i32>>
+ #dlti.dl_entry<"max_vector_op_width", "128">>
>} {}
// -----
@@ -77,17 +77,17 @@ module attributes {
// CHECK: module attributes {
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 64 : i64>>,
+// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "64">>,
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i64>>
+// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">>
// CHECK-SAME: >} {
// CHECK: }
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 64: i64>>,
+ #dlti.dl_entry<"max_vector_op_width", "64">>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128: i64>>
+ #dlti.dl_entry<"max_vector_op_width", "128">>
>} {}
// -----
@@ -95,15 +95,15 @@ module attributes {
// CHECK: module attributes {
// CHECK-SAME: dlti.target_system_spec = #dlti.target_system_spec<
// CHECK-SAME: "CPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 64 : i64>>,
+// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "64">>,
// CHECK-SAME: "GPU" : #dlti.target_device_spec<
-// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i64>>
+// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">>
// CHECK-SAME: >} {
// CHECK: }
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 64>>,
+ #dlti.dl_entry<"max_vector_op_width", "64">>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128>>
+ #dlti.dl_entry<"max_vector_op_width", "128">>
>} {}
diff --git a/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp b/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp
index bcbfa88c5d815..2f1ea548bea90 100644
--- a/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp
+++ b/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp
@@ -495,11 +495,11 @@ TEST(DataLayout, NullSpec) {
EXPECT_EQ(layout.getGlobalMemorySpace(), Attribute());
EXPECT_EQ(layout.getStackAlignment(), 0u);
- EXPECT_EQ(layout.getDevicePropertyValueAsInt(
+ EXPECT_EQ(layout.getDevicePropertyValue(
Builder(&ctx).getStringAttr("CPU" /* device ID*/),
Builder(&ctx).getStringAttr("L1_cache_size_in_bytes")),
std::nullopt);
- EXPECT_EQ(layout.getDevicePropertyValueAsInt(
+ EXPECT_EQ(layout.getDevicePropertyValue(
Builder(&ctx).getStringAttr("CPU" /* device ID*/),
Builder(&ctx).getStringAttr("max_vector_width")),
std::nullopt);
@@ -535,11 +535,11 @@ TEST(DataLayout, EmptySpec) {
EXPECT_EQ(layout.getGlobalMemorySpace(), Attribute());
EXPECT_EQ(layout.getStackAlignment(), 0u);
- EXPECT_EQ(layout.getDevicePropertyValueAsInt(
+ EXPECT_EQ(layout.getDevicePropertyValue(
Builder(&ctx).getStringAttr("CPU" /* device ID*/),
Builder(&ctx).getStringAttr("L1_cache_size_in_bytes")),
std::nullopt);
- EXPECT_EQ(layout.getDevicePropertyValueAsInt(
+ EXPECT_EQ(layout.getDevicePropertyValue(
Builder(&ctx).getStringAttr("CPU" /* device ID*/),
Builder(&ctx).getStringAttr("max_vector_width")),
std::nullopt);
@@ -599,8 +599,8 @@ TEST(DataLayout, SpecWithTargetSystemDescEntries) {
module attributes { dl_target_sys_desc_test.target_system_spec =
#dl_target_sys_desc_test.target_system_spec<
"CPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : ui32>,
- #dlti.dl_entry<"max_vector_op_width", 128 : ui32>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">,
+ #dlti.dl_entry<"max_vector_op_width", "128">>
> } {}
)MLIR";
@@ -610,14 +610,14 @@ TEST(DataLayout, SpecWithTargetSystemDescEntries) {
OwningOpRef<ModuleOp> module = parseSourceString<ModuleOp>(ir, &ctx);
DataLayout layout(*module);
- EXPECT_EQ(layout.getDevicePropertyValueAsInt(
+ EXPECT_EQ(layout.getDevicePropertyValue(
Builder(&ctx).getStringAttr("CPU") /* device ID*/,
Builder(&ctx).getStringAttr("L1_cache_size_in_bytes")),
- std::optional<int64_t>(4096));
- EXPECT_EQ(layout.getDevicePropertyValueAsInt(
+ std::optional<StringAttr>(Builder(&ctx).getStringAttr("4096")));
+ EXPECT_EQ(layout.getDevicePropertyValue(
Builder(&ctx).getStringAttr("CPU") /* device ID*/,
Builder(&ctx).getStringAttr("max_vector_op_width")),
- std::optional<int64_t>(128));
+ std::optional<StringAttr>(Builder(&ctx).getStringAttr("128")));
}
TEST(DataLayout, Caching) {
``````````
</details>
https://github.com/llvm/llvm-project/pull/96706
More information about the Mlir-commits
mailing list