DLTI: Simplifying getDevicePropertyValue API by returning Attribute t…

…ype value (llvm#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`.
llvm-project-tlp · Jun 27, 2024 · 849cea9 · 849cea9
1 parent eaec0bf
commit 849cea9
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 40 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) {