diff options
author | Niranjan Hasabnis <niranjan.hasabnis@intel.com> | 2024-06-27 01:24:05 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-06-27 09:24:05 +0100 |
commit | bdeee9b105b7f1e75adcbfbd43f884d4ddb1a612 (patch) | |
tree | e969d0fdf7a4e01a532b080982d38dbc05a48b91 | |
parent | 7a969ec1e114b6674c08e82ca048a3c4576bf0dd (diff) | |
download | llvm-bdeee9b105b7f1e75adcbfbd43f884d4ddb1a612.zip llvm-bdeee9b105b7f1e75adcbfbd43f884d4ddb1a612.tar.gz llvm-bdeee9b105b7f1e75adcbfbd43f884d4ddb1a612.tar.bz2 |
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`.
-rw-r--r-- | mlir/include/mlir/Interfaces/DataLayoutInterfaces.h | 9 | ||||
-rw-r--r-- | mlir/include/mlir/Interfaces/DataLayoutInterfaces.td | 6 | ||||
-rw-r--r-- | mlir/lib/Dialect/DLTI/DLTI.cpp | 1 | ||||
-rw-r--r-- | mlir/lib/Interfaces/DataLayoutInterfaces.cpp | 13 | ||||
-rw-r--r-- | mlir/test/Dialect/DLTI/invalid.mlir | 2 | ||||
-rw-r--r-- | mlir/test/Dialect/DLTI/roundtrip.mlir | 4 | ||||
-rw-r--r-- | mlir/test/Dialect/DLTI/valid.mlir | 64 | ||||
-rw-r--r-- | mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp | 20 |
8 files changed, 79 insertions, 40 deletions
diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h index 4cbad38..ab65f92 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 @@ public: /// 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 8ff0e3f..d4b8999 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 592fced..420c605 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 df86bd7..2634245 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 d732fb4..36bf608 100644 --- a/mlir/test/Dialect/DLTI/invalid.mlir +++ b/mlir/test/Dialect/DLTI/invalid.mlir @@ -126,7 +126,7 @@ module attributes { // expected-error@+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 7b8255a..43188aa 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 98e9d0b..8ee7a7e 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 bcbfa88..d1227b0 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) { |