[Mlir-commits] [mlir] bdeee9b - DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value (#96706)
llvmlistbot at llvm.org
llvmlistbot at llvm.org
Thu Jun 27 01:24:11 PDT 2024
Author: Niranjan Hasabnis
Date: 2024-06-27T09:24:05+01:00
New Revision: bdeee9b105b7f1e75adcbfbd43f884d4ddb1a612
URL: https://github.com/llvm/llvm-project/commit/bdeee9b105b7f1e75adcbfbd43f884d4ddb1a612
DIFF: https://github.com/llvm/llvm-project/commit/bdeee9b105b7f1e75adcbfbd43f884d4ddb1a612.diff
LOG: DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value (#96706)
**Rationale**
- With the current flexibility of supporting any type of value, we will
need to offer type-specific APIs to fetch a value (e.g.,
`getDevicePropertyValueAsInt` for integer type,
`getDevicePropertyValueAsFloat` for float type, etc.) A single type of
value will eliminate this need.
- Current flexibility can also lead to typing errors when a user fetches
the value of a property using an API that is not consistent with the
type of the value.
**What is the change**
For following system description,
```
module attributes {
dlti.target_system_spec = #dlti.target_system_spec<
"CPU": #dlti.target_device_spec<
#dlti.dl_entry<"max_vector_op_width", 64.0 : f32>>,
"GPU": #dlti.target_device_spec<
#dlti.dl_entry<"max_vector_op_width", 128 : ui32>>
>} {}
```
a user no longer needs to use `getDevicePropertyValueAsInt` for
retrieving GPU's `max_vector_op_width` and
`getDevicePropertyValueAsFloat` for retrieving CPU's
`max_vector_op_width`. Instead it can be done with a uniform API of
`getDevicePropertyValue`.
Added:
Modified:
mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
mlir/include/mlir/Interfaces/DataLayoutInterfaces.td
mlir/lib/Dialect/DLTI/DLTI.cpp
mlir/lib/Interfaces/DataLayoutInterfaces.cpp
mlir/test/Dialect/DLTI/invalid.mlir
mlir/test/Dialect/DLTI/roundtrip.mlir
mlir/test/Dialect/DLTI/valid.mlir
mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp
Removed:
################################################################################
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
index 4cbad38df42ba..ab65f92820a6a 100644
--- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
+++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h
@@ -93,8 +93,7 @@ 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<Attribute> 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 +245,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<Attribute>
+ 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..d4b89997d9c86 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<Attribute>",
+ /*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..420c605d1a19b 100644
--- a/mlir/lib/Dialect/DLTI/DLTI.cpp
+++ b/mlir/lib/Dialect/DLTI/DLTI.cpp
@@ -352,6 +352,7 @@ 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();
diff --git a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp
index df86bd757b628..2634245a4b7b1 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<Attribute>
+mlir::detail::getDevicePropertyValue(DataLayoutEntryInterface entry) {
if (entry == DataLayoutEntryInterface())
return std::nullopt;
- auto value = cast<IntegerAttr>(entry.getValue());
- return value.getValue().getZExtValue();
+ return entry.getValue();
}
DataLayoutEntryList
@@ -661,7 +660,7 @@ uint64_t mlir::DataLayout::getStackAlignment() const {
return *stackAlignment;
}
-std::optional<int64_t> mlir::DataLayout::getDevicePropertyValueAsInt(
+std::optional<Attribute> 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..36bf6088fab7e 100644
--- a/mlir/test/Dialect/DLTI/invalid.mlir
+++ b/mlir/test/Dialect/DLTI/invalid.mlir
@@ -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 : i32>>
>} {}
// -----
diff --git a/mlir/test/Dialect/DLTI/roundtrip.mlir b/mlir/test/Dialect/DLTI/roundtrip.mlir
index 7b8255ad71d4d..43188aad595a7 100644
--- a/mlir/test/Dialect/DLTI/roundtrip.mlir
+++ b/mlir/test/Dialect/DLTI/roundtrip.mlir
@@ -66,8 +66,8 @@
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 : ui32>>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"dlti.max_vector_op_width", 128: ui32>>
+ #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>>
>} {}
diff --git a/mlir/test/Dialect/DLTI/valid.mlir b/mlir/test/Dialect/DLTI/valid.mlir
index 98e9d0b8de491..8ee7a7eaa069d 100644
--- a/mlir/test/Dialect/DLTI/valid.mlir
+++ b/mlir/test/Dialect/DLTI/valid.mlir
@@ -13,9 +13,9 @@
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 : i32>>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128: i32>>
+ #dlti.dl_entry<"max_vector_op_width", 128 : i32>>
>} {}
// -----
@@ -31,9 +31,9 @@ module attributes {
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 : i32>>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 8192: i32>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i32>>
>} {}
// -----
@@ -49,9 +49,9 @@ module attributes {
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 : i64>>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"L1_cache_size_in_bytes", 8192: i64>>
+ #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i64>>
>} {}
// -----
@@ -67,9 +67,9 @@ module attributes {
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 : i32>>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128: i32>>
+ #dlti.dl_entry<"max_vector_op_width", 128 : i32>>
>} {}
// -----
@@ -85,9 +85,9 @@ module attributes {
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 : i64>>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128: i64>>
+ #dlti.dl_entry<"max_vector_op_width", 128 : i64>>
>} {}
// -----
@@ -103,7 +103,47 @@ module attributes {
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 : i64>>,
"GPU": #dlti.target_device_spec<
- #dlti.dl_entry<"max_vector_op_width", 128>>
+ #dlti.dl_entry<"max_vector_op_width", 128 : i64>>
+ >} {}
+
+// -----
+
+// Check values of mixed type
+//
+// 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 : ui32>>,
+// CHECK-SAME: "GPU" : #dlti.target_device_spec<
+// 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 : ui32>>,
+ "GPU": #dlti.target_device_spec<
+ #dlti.dl_entry<"max_vector_op_width", "128">>
+ >} {}
+
+// -----
+
+// Check values of mixed type
+//
+// 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", 4.096000e+03 : f32>>,
+// CHECK-SAME: "GPU" : #dlti.target_device_spec<
+// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "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", 4096.0 : f32>>,
+ "GPU": #dlti.target_device_spec<
+ #dlti.dl_entry<"L1_cache_size_in_bytes", "128">>
>} {}
diff --git a/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp b/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp
index bcbfa88c5d815..d1227b045d4ed 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<Attribute>(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<Attribute>(Builder(&ctx).getStringAttr("128")));
}
TEST(DataLayout, Caching) {
More information about the Mlir-commits
mailing list