DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value #96706

nhasabni · 2024-06-25T22:11:34Z

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.

llvmbot · 2024-06-25T22:12:05Z

@llvm/pr-subscribers-mlir-dlti

@llvm/pr-subscribers-mlir

Author: Niranjan Hasabnis (nhasabni)

Changes

So instead of

 module attributes {
   dlti.target_system_spec = #dlti.target_system_spec&lt;
     "CPU": #dlti.target_device_spec&lt;
      #dlti.dl_entry&lt;"max_vector_op_width", 64&gt;&gt;,
     "GPU": #dlti.target_device_spec&lt;
      #dlti.dl_entry&lt;"max_vector_op_width", 128&gt;&gt;
   &gt;} {}

this PR supports following:

 module attributes {
   dlti.target_system_spec = #dlti.target_system_spec&lt;
     "CPU": #dlti.target_device_spec&lt;
      #dlti.dl_entry&lt;"max_vector_op_width", "64"&gt;&gt;,
     "GPU": #dlti.target_device_spec&lt;
      #dlti.dl_entry&lt;"max_vector_op_width", "128"&gt;&gt;
   &gt;} {}

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 --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@+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@+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@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@+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@+4 {{dlti.target_device_spec supports values of StringAttr type only.}}
+  // expected-error@+5 {{Error in parsing target device spec}}
+  // expected-error@+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) {

joker-eph · 2024-06-25T22:18:24Z

Please describe the context / motivation in the description (you're nicely saying "what" you're doing here, but not "why", which is in general the most important aspect of the description)

nhasabni · 2024-06-25T22:31:42Z

Please describe the context / motivation in the description (you're nicely saying "what" you're doing here, but not "why", which is in general the most important aspect of the description)

I should have done that before. Sorry. I have updated the description now.

joker-eph · 2024-06-25T22:42:01Z

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.

Return an Attribute also eliminates this need though.

nhasabni · 2024-06-25T22:47:39Z

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.

Return an Attribute also eliminates this need though.

Hmm, agree. @rengolin any thoughts?

rengolin · 2024-06-26T08:43:34Z

Return an Attribute also eliminates this need though.

Hmm, agree. @rengolin any thoughts?

That would probably be simpler, yeah.

…StringAttr value type

nhasabni · 2024-06-26T17:04:14Z

@joker-eph Returning the value of the property as Attribute now. No more restriction on the type of value supported in the description. Pls check.

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

DLTI: Restricting value type to StringAttr in a target device spec

e027976

llvmbot added mlir mlir:dlti labels Jun 25, 2024

rengolin mentioned this pull request Jun 26, 2024

Next steps on target descriptor plaidml/tpp-mlir#934

Open

10 tasks

Using Attribute as the return type of value; removing restriction of …

970e716

…StringAttr value type

nhasabni changed the title ~~DLTI: Restricting value type to StringAttr in a target device spec~~ DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value Jun 26, 2024

Removing spurious changes introduced by first commit

43352c1

joker-eph approved these changes Jun 26, 2024

View reviewed changes

rengolin requested a review from ftynse June 27, 2024 08:23

rengolin merged commit bdeee9b into llvm:main Jun 27, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value #96706

DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value #96706

nhasabni commented Jun 25, 2024 •

edited

Loading

llvmbot commented Jun 25, 2024 •

edited

Loading

joker-eph commented Jun 25, 2024 •

edited

Loading

nhasabni commented Jun 25, 2024

joker-eph commented Jun 25, 2024

nhasabni commented Jun 25, 2024

rengolin commented Jun 26, 2024

nhasabni commented Jun 26, 2024

DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value #96706

DLTI: Simplifying getDevicePropertyValue API by returning Attribute type value #96706

Conversation

nhasabni commented Jun 25, 2024 • edited Loading

llvmbot commented Jun 25, 2024 • edited Loading

joker-eph commented Jun 25, 2024 • edited Loading

nhasabni commented Jun 25, 2024

joker-eph commented Jun 25, 2024

nhasabni commented Jun 25, 2024

rengolin commented Jun 26, 2024

nhasabni commented Jun 26, 2024

nhasabni commented Jun 25, 2024 •

edited

Loading

llvmbot commented Jun 25, 2024 •

edited

Loading

joker-eph commented Jun 25, 2024 •

edited

Loading