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`.

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) {