From e02797697aa57edabd9ad5ca9cd12315fe27f341 Mon Sep 17 00:00:00 2001 From: Niranjan Hasabnis Date: Tue, 25 Jun 2024 15:07:34 -0700 Subject: [PATCH 1/3] DLTI: Restricting value type to StringAttr in a target device spec --- .../mlir/Interfaces/DataLayoutInterfaces.h | 10 ++-- .../mlir/Interfaces/DataLayoutInterfaces.td | 6 +-- mlir/lib/Dialect/DLTI/DLTI.cpp | 6 +++ mlir/lib/Interfaces/DataLayoutInterfaces.cpp | 13 +++-- mlir/test/Dialect/DLTI/invalid.mlir | 25 +++++++--- mlir/test/Dialect/DLTI/roundtrip.mlir | 8 ++-- mlir/test/Dialect/DLTI/valid.mlir | 48 +++++++++---------- .../Interfaces/DataLayoutInterfacesTest.cpp | 20 ++++---- 8 files changed, 77 insertions(+), 59 deletions(-) diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h index 4cbad38df42bab..ec90c82131246e 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 -getDevicePropertyValueAsInt(DataLayoutEntryInterface entry); +std::optional +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 - getDevicePropertyValueAsInt(TargetSystemSpecInterface::DeviceID, - StringAttr propertyName) const; + std::optional + 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 8ff0e3f5f863b6..4975e36551a422 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", - /*methodName=*/"getDevicePropertyValueAsInt", + /*retTy=*/"std::optional", + /*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 592fced0b2abdd..080cba397b5493 100644 --- a/mlir/lib/Dialect/DLTI/DLTI.cpp +++ b/mlir/lib/Dialect/DLTI/DLTI.cpp @@ -352,9 +352,15 @@ TargetDeviceSpecAttr::verify(function_ref 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(); 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(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 df86bd757b6282..b8bbe27fa15dd1 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 -mlir::detail::getDevicePropertyValueAsInt(DataLayoutEntryInterface entry) { +std::optional +mlir::detail::getDevicePropertyValue(DataLayoutEntryInterface entry) { if (entry == DataLayoutEntryInterface()) return std::nullopt; - auto value = cast(entry.getValue()); - return value.getValue().getZExtValue(); + return cast(entry.getValue()); } DataLayoutEntryList @@ -661,7 +660,7 @@ uint64_t mlir::DataLayout::getStackAlignment() const { return *stackAlignment; } -std::optional mlir::DataLayout::getDevicePropertyValueAsInt( +std::optional mlir::DataLayout::getDevicePropertyValue( TargetSystemSpecInterface::DeviceID deviceID, StringAttr propertyName) const { checkValid(); @@ -676,9 +675,9 @@ std::optional 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(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 d732fb462e68e9..5eb9559e5d415b 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`}} 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`}} 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`}} 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`}} + 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 7b8255ad71d4d9..c5be3bc8053304 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 98e9d0b8de4913..2084168d865dfc 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 bcbfa88c5d8159..2f1ea548bea904 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 module = parseSourceString(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(4096)); - EXPECT_EQ(layout.getDevicePropertyValueAsInt( + std::optional(Builder(&ctx).getStringAttr("4096"))); + EXPECT_EQ(layout.getDevicePropertyValue( Builder(&ctx).getStringAttr("CPU") /* device ID*/, Builder(&ctx).getStringAttr("max_vector_op_width")), - std::optional(128)); + std::optional(Builder(&ctx).getStringAttr("128"))); } TEST(DataLayout, Caching) { From 970e7168f1ef4fb69897bae948ee41d505cdc536 Mon Sep 17 00:00:00 2001 From: Niranjan Hasabnis Date: Wed, 26 Jun 2024 10:00:30 -0700 Subject: [PATCH 2/3] Using Attribute as the return type of value; removing restriction of StringAttr value type --- .../mlir/Interfaces/DataLayoutInterfaces.h | 5 ++- .../mlir/Interfaces/DataLayoutInterfaces.td | 2 +- mlir/lib/Dialect/DLTI/DLTI.cpp | 5 --- mlir/lib/Interfaces/DataLayoutInterfaces.cpp | 6 ++-- mlir/test/Dialect/DLTI/invalid.mlir | 13 ------- mlir/test/Dialect/DLTI/valid.mlir | 36 +++++++++++++++++++ .../Interfaces/DataLayoutInterfacesTest.cpp | 4 +-- 7 files changed, 44 insertions(+), 27 deletions(-) diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.h index ec90c82131246e..ab65f92820a6a8 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 -getDevicePropertyValue(DataLayoutEntryInterface entry); +std::optional 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,7 +245,7 @@ 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 + std::optional getDevicePropertyValue(TargetSystemSpecInterface::DeviceID, StringAttr propertyName) const; diff --git a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td index 4975e36551a422..d4b89997d9c867 100644 --- a/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td +++ b/mlir/include/mlir/Interfaces/DataLayoutInterfaces.td @@ -468,7 +468,7 @@ 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", + /*retTy=*/"std::optional", /*methodName=*/"getDevicePropertyValue", /*args=*/(ins "::mlir::DataLayoutEntryInterface":$entry), /*methodBody=*/"", diff --git a/mlir/lib/Dialect/DLTI/DLTI.cpp b/mlir/lib/Dialect/DLTI/DLTI.cpp index 080cba397b5493..420c605d1a19b2 100644 --- a/mlir/lib/Dialect/DLTI/DLTI.cpp +++ b/mlir/lib/Dialect/DLTI/DLTI.cpp @@ -356,11 +356,6 @@ TargetDeviceSpecAttr::verify(function_ref emitError, auto id = entry.getKey().get(); 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(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 b8bbe27fa15dd1..2634245a4b7b1e 100644 --- a/mlir/lib/Interfaces/DataLayoutInterfaces.cpp +++ b/mlir/lib/Interfaces/DataLayoutInterfaces.cpp @@ -293,12 +293,12 @@ mlir::detail::getDefaultStackAlignment(DataLayoutEntryInterface entry) { return value.getValue().getZExtValue(); } -std::optional +std::optional mlir::detail::getDevicePropertyValue(DataLayoutEntryInterface entry) { if (entry == DataLayoutEntryInterface()) return std::nullopt; - return cast(entry.getValue()); + return entry.getValue(); } DataLayoutEntryList @@ -660,7 +660,7 @@ uint64_t mlir::DataLayout::getStackAlignment() const { return *stackAlignment; } -std::optional mlir::DataLayout::getDevicePropertyValue( +std::optional mlir::DataLayout::getDevicePropertyValue( TargetSystemSpecInterface::DeviceID deviceID, StringAttr propertyName) const { checkValid(); diff --git a/mlir/test/Dialect/DLTI/invalid.mlir b/mlir/test/Dialect/DLTI/invalid.mlir index 5eb9559e5d415b..8d7d7e92315d27 100644 --- a/mlir/test/Dialect/DLTI/invalid.mlir +++ b/mlir/test/Dialect/DLTI/invalid.mlir @@ -155,16 +155,3 @@ module attributes { #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`}} - 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/valid.mlir b/mlir/test/Dialect/DLTI/valid.mlir index 2084168d865dfc..8f171faf23eb68 100644 --- a/mlir/test/Dialect/DLTI/valid.mlir +++ b/mlir/test/Dialect/DLTI/valid.mlir @@ -107,3 +107,39 @@ module attributes { "GPU": #dlti.target_device_spec< #dlti.dl_entry<"max_vector_op_width", "128">> >} {} + +// ----- + +// 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: 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 2f1ea548bea904..d1227b045d4ed3 100644 --- a/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp +++ b/mlir/unittests/Interfaces/DataLayoutInterfacesTest.cpp @@ -613,11 +613,11 @@ TEST(DataLayout, SpecWithTargetSystemDescEntries) { EXPECT_EQ(layout.getDevicePropertyValue( Builder(&ctx).getStringAttr("CPU") /* device ID*/, Builder(&ctx).getStringAttr("L1_cache_size_in_bytes")), - std::optional(Builder(&ctx).getStringAttr("4096"))); + std::optional(Builder(&ctx).getStringAttr("4096"))); EXPECT_EQ(layout.getDevicePropertyValue( Builder(&ctx).getStringAttr("CPU") /* device ID*/, Builder(&ctx).getStringAttr("max_vector_op_width")), - std::optional(Builder(&ctx).getStringAttr("128"))); + std::optional(Builder(&ctx).getStringAttr("128"))); } TEST(DataLayout, Caching) { From 43352c1e3a8c2fa297678efa7ead455c9f5eafba Mon Sep 17 00:00:00 2001 From: Niranjan Hasabnis Date: Wed, 26 Jun 2024 10:18:28 -0700 Subject: [PATCH 3/3] Removing spurious changes introduced by first commit --- mlir/test/Dialect/DLTI/invalid.mlir | 12 +++---- mlir/test/Dialect/DLTI/roundtrip.mlir | 8 ++--- mlir/test/Dialect/DLTI/valid.mlir | 52 ++++++++++++++------------- 3 files changed, 38 insertions(+), 34 deletions(-) diff --git a/mlir/test/Dialect/DLTI/invalid.mlir b/mlir/test/Dialect/DLTI/invalid.mlir index 8d7d7e92315d27..36bf6088fab7e9 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`}} dlti.target_system_spec = #dlti.target_system_spec< : #dlti.target_device_spec< - #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">> + #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>> >} {} // ----- @@ -126,7 +126,7 @@ module attributes { // expected-error@+2 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef`}} dlti.target_system_spec = #dlti.target_system_spec< 0: #dlti.target_device_spec< - #dlti.dl_entry<"L1_cache_size_in_bytes", "4096">> + #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>> >} {} // ----- @@ -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,6 @@ module attributes { // expected-error@+5 {{failed to parse DLTI_TargetSystemSpecAttr parameter 'entries' which is to be a `::llvm::ArrayRef`}} 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>> >} {} diff --git a/mlir/test/Dialect/DLTI/roundtrip.mlir b/mlir/test/Dialect/DLTI/roundtrip.mlir index c5be3bc8053304..43188aad595a7a 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">>, +// CHECK: #dlti.dl_entry<"dlti.L1_cache_size_in_bytes", 4096 : ui32>>, // CHECK: "GPU" : #dlti.target_device_spec< -// CHECK: #dlti.dl_entry<"dlti.max_vector_op_width", "128">> +// CHECK: #dlti.dl_entry<"dlti.max_vector_op_width", 128 : ui32>> // 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">>, + #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">> + #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 8f171faf23eb68..8ee7a7eaa069db 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">>, +// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>>, // CHECK-SAME: "GPU" : #dlti.target_device_spec< -// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">> +// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i32>> // 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">>, + #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>>, "GPU": #dlti.target_device_spec< - #dlti.dl_entry<"max_vector_op_width", "128">> + #dlti.dl_entry<"max_vector_op_width", 128 : i32>> >} {} // ----- @@ -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">>, +// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>>, // CHECK-SAME: "GPU" : #dlti.target_device_spec< -// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">> +// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i32>> // 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">>, + #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i32>>, "GPU": #dlti.target_device_spec< - #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">> + #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i32>> >} {} // ----- @@ -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">>, +// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i64>>, // CHECK-SAME: "GPU" : #dlti.target_device_spec< -// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">> +// CHECK-SAME: #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i64>> // 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">>, + #dlti.dl_entry<"L1_cache_size_in_bytes", 4096 : i64>>, "GPU": #dlti.target_device_spec< - #dlti.dl_entry<"L1_cache_size_in_bytes", "8192">> + #dlti.dl_entry<"L1_cache_size_in_bytes", 8192 : i64>> >} {} // ----- @@ -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">>, +// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 64 : i32>>, // CHECK-SAME: "GPU" : #dlti.target_device_spec< -// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">> +// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i32>> // 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 : i32>>, "GPU": #dlti.target_device_spec< - #dlti.dl_entry<"max_vector_op_width", "128">> + #dlti.dl_entry<"max_vector_op_width", 128 : i32>> >} {} // ----- @@ -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">>, +// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 64 : i64>>, // CHECK-SAME: "GPU" : #dlti.target_device_spec< -// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">> +// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i64>> // 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 : i64>>, "GPU": #dlti.target_device_spec< - #dlti.dl_entry<"max_vector_op_width", "128">> + #dlti.dl_entry<"max_vector_op_width", 128 : i64>> >} {} // ----- @@ -95,21 +95,23 @@ 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">>, +// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 64 : i64>>, // CHECK-SAME: "GPU" : #dlti.target_device_spec< -// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", "128">> +// CHECK-SAME: #dlti.dl_entry<"max_vector_op_width", 128 : i64>> // 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 : 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< @@ -128,6 +130,8 @@ module attributes { // ----- +// 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<