Addressing review comments - removing methods for L1 size and max vector width

Author: nhasabni

mlir/include/mlir/Dialect/DLTI/DLTIAttrs.td

@@ -183,13 +183,6 @@ def DLTI_TargetDeviceSpecAttr :
   let mnemonic = "target_device_spec";
   let genVerifyDecl = 1;
   let assemblyFormat = "`<` $entries `>`";
-  let extraClassDeclaration = [{
-    /// Returns max vector op width identifier.
-    StringAttr getMaxVectorOpWidthIdentifier();
-
-    /// Returns L1 cache size identifier
-    StringAttr getL1CacheSizeInBytesIdentifier();
-  }];
 }
 
 #endif  // MLIR_DIALECT_DLTI_DLTIATTRS_TD

mlir/include/mlir/Dialect/DLTI/DLTIBase.td

@@ -55,13 +55,6 @@ def DLTI_Dialect : Dialect {
 
     constexpr const static ::llvm::StringLiteral
     kDataLayoutStackAlignmentKey = "dlti.stack_alignment";
-
-    // Constants used in target description part of DLTI.
-    constexpr const static ::llvm::StringLiteral
-    kTargetDeviceMaxVectorOpWidthKey = "dlti.max_vector_op_width";
-
-    constexpr const static ::llvm::StringLiteral
-    kTargetDeviceL1CacheSizeInBytesKey = "dlti.L1_cache_size_in_bytes";
   }];
 
   let useDefaultAttributePrinterParser = 1;

mlir/include/mlir/Interfaces/DataLayoutInterfaces.h

@@ -91,13 +91,10 @@ Attribute getDefaultGlobalMemorySpace(DataLayoutEntryInterface entry);
 /// DataLayoutInterface if specified, otherwise returns the default.
 uint64_t getDefaultStackAlignment(DataLayoutEntryInterface entry);
 
-/// Return max vector op width from the specified DataLayoutEntry. If the
-/// property is missing from the entry, then return std::nullopt.
-std::optional<int64_t> getMaxVectorOpWidth(DataLayoutEntryInterface entry);
-
-/// Return L1 cache size in bytes from the specified DataLayoutEntry. If the
-/// property is missing from the entry, then return std::nullopt.
-std::optional<int64_t> getL1CacheSizeInBytes(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);
 
 /// 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
@@ -247,15 +244,11 @@ class DataLayout {
   /// unspecified.
   uint64_t getStackAlignment() const;
 
-  /// Returns for max vector op width if the property is defined for the given
-  /// device ID, otherwise return std::nullopt.
-  std::optional<int64_t>
-      getMaxVectorOpWidth(TargetSystemSpecInterface::DeviceID) const;
-
-  /// Returns for L1 cache size if the property is defined for the given
-  /// device ID, otherwise return std::nullopt.
+  /// 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>
-      getL1CacheSizeInBytes(TargetSystemSpecInterface::DeviceID) const;
+  getDevicePropertyValueAsInt(TargetSystemSpecInterface::DeviceID,
+                              StringAttr propertyName) const;
 
 private:
   /// Combined layout spec at the given scope.

mlir/include/mlir/Interfaces/DataLayoutInterfaces.td

@@ -237,21 +237,7 @@ def TargetDeviceSpecInterface : AttrInterface<"TargetDeviceSpecInterface"> {
       /*args=*/(ins "::mlir::Location":$loc),
       /*methodBody=*/"",
       /*defaultImplementation=*/[{ return ::mlir::success(); }]
-    >,
-    InterfaceMethod<
-      /*description=*/"Returns max vector op width identifier. ",
-      /*retTy=*/"::mlir::StringAttr",
-      /*methodName=*/"getMaxVectorOpWidthIdentifier",
-      /*args=*/(ins),
-      /*methodBody=*/""
-    >,
-    InterfaceMethod<
-      /*description=*/"Returns L1 cache size identifier identifier. ",
-      /*retTy=*/"::mlir::StringAttr",
-      /*methodName=*/"getL1CacheSizeInBytesIdentifier",
-      /*args=*/(ins),
-      /*methodBody=*/""
-    >,
+    >
   ];
 }
 
@@ -480,25 +466,14 @@ def DataLayoutOpInterface : OpInterface<"DataLayoutOpInterface"> {
       }]
     >,
     StaticInterfaceMethod<
-      /*description=*/"Returns the max vector op width, if the property is "
-                      "defined. Otherwise, it returns std::nullopt.",
-      /*retTy=*/"std::optional<int64_t>",
-      /*methodName=*/"getMaxVectorOpWidth",
-      /*args=*/(ins "::mlir::DataLayoutEntryInterface":$entry),
-      /*methodBody=*/"",
-      /*defaultImplementation=*/[{
-        return ::mlir::detail::getMaxVectorOpWidth(entry);
-      }]
-    >,
-    StaticInterfaceMethod<
-      /*description=*/"Returns the L1 cache size in bytes, if the property is "
+      /*description=*/"Returns the value of the property, if the property is "
                       "defined. Otherwise, it returns std::nullopt.",
       /*retTy=*/"std::optional<int64_t>",
-      /*methodName=*/"getL1CacheSizeInBytes",
+      /*methodName=*/"getDevicePropertyValueAsInt",
       /*args=*/(ins "::mlir::DataLayoutEntryInterface":$entry),
       /*methodBody=*/"",
       /*defaultImplementation=*/[{
-        return ::mlir::detail::getL1CacheSizeInBytes(entry);
+        return ::mlir::detail::getDevicePropertyValueAsInt(entry);
       }]
     >
   ];

mlir/lib/Dialect/DLTI/DLTI.cpp

@@ -356,42 +356,11 @@ TargetDeviceSpecAttr::verify(function_ref<InFlightDiagnostic()> emitError,
       if (!ids.insert(id).second)
         return emitError() << "repeated layout entry key: " << id.getValue();
     }
-
-    // Check that required keys are of right type.
-    StringRef entryName = entry.getKey().get<StringAttr>().strref();
-    if (entryName == DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey) {
-      IntegerAttr value =
-          llvm::dyn_cast_if_present<IntegerAttr>(entry.getValue());
-      if (!value || !value.getType().isInteger())
-        return emitError() << "target_device_spec requires value of key: "
-                           << DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey
-                           << " to be of integer type";
-    } else if (entryName == DLTIDialect::kTargetDeviceMaxVectorOpWidthKey) {
-      IntegerAttr value =
-          llvm::dyn_cast_if_present<IntegerAttr>(entry.getValue());
-      if (!value || !value.getType().isInteger())
-        return emitError() << "target_device_spec requires value of key: "
-                           << DLTIDialect::kTargetDeviceMaxVectorOpWidthKey
-                           << " to be of integer type";
-    } else {
-      return emitError() << "unknown target device spec key name: "
-                         << entryName;
-    }
   }
 
   return success();
 }
 
-StringAttr TargetDeviceSpecAttr::getMaxVectorOpWidthIdentifier() {
-  return Builder(getContext())
-      .getStringAttr(DLTIDialect::kTargetDeviceMaxVectorOpWidthKey);
-}
-
-StringAttr TargetDeviceSpecAttr::getL1CacheSizeInBytesIdentifier() {
-  return Builder(getContext())
-      .getStringAttr(DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey);
-}
-
 //===----------------------------------------------------------------------===//
 // TargetSystemSpecAttr
 //===----------------------------------------------------------------------===//
@@ -457,34 +426,12 @@ class TargetDataLayoutInterface : public DataLayoutDialectInterface {
 };
 } // namespace
 
-namespace {
-/// An interface to check entries of a target device spec.
-class SystemDescSpecInterface : public DataLayoutDialectInterface {
-public:
-  using DataLayoutDialectInterface::DataLayoutDialectInterface;
-
-  LogicalResult verifyEntry(TargetDeviceSpecInterface entry,
-                            Location loc) const final {
-
-    for (DataLayoutEntryInterface dl_entry : entry.getEntries()) {
-      StringRef entryName = dl_entry.getKey().get<StringAttr>().strref();
-      // Check that the key name is known to us. Although, we may allow keys
-      // unknown to us.
-      if (entryName != DLTIDialect::kTargetDeviceMaxVectorOpWidthKey &&
-          entryName != DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey)
-        return emitError(loc) << "unknown target desc key name: " << entryName;
-    }
-    return success();
-  }
-};
-} // namespace
-
 void DLTIDialect::initialize() {
   addAttributes<
 #define GET_ATTRDEF_LIST
 #include "mlir/Dialect/DLTI/DLTIAttrs.cpp.inc"
       >();
-  addInterfaces<TargetDataLayoutInterface, SystemDescSpecInterface>();
+  addInterfaces<TargetDataLayoutInterface>();
 }
 
 LogicalResult DLTIDialect::verifyOperationAttribute(Operation *op,

mlir/lib/Interfaces/DataLayoutInterfaces.cpp

@@ -293,21 +293,8 @@ mlir::detail::getDefaultStackAlignment(DataLayoutEntryInterface entry) {
   return value.getValue().getZExtValue();
 }
 
-// Returns the max vector op width if specified in the given entry. If the entry
-// is empty (meaning the spec is missing), returns std::nullopt.
 std::optional<int64_t>
-mlir::detail::getMaxVectorOpWidth(DataLayoutEntryInterface entry) {
-  if (entry == DataLayoutEntryInterface())
-    return std::nullopt;
-
-  auto value = cast<IntegerAttr>(entry.getValue());
-  return value.getValue().getZExtValue();
-}
-
-// Returns the L1 cache size if specified in the given entry. If the entry
-// is empty (meaning the spec is missing), returns std::nullopt.
-std::optional<int64_t>
-mlir::detail::getL1CacheSizeInBytes(DataLayoutEntryInterface entry) {
+mlir::detail::getDevicePropertyValueAsInt(DataLayoutEntryInterface entry) {
   if (entry == DataLayoutEntryInterface())
     return std::nullopt;
 
@@ -348,15 +335,12 @@ static DataLayoutSpecInterface getSpec(Operation *operation) {
 
 static TargetSystemSpecInterface getTargetSystemSpec(Operation *operation) {
   if (operation) {
-    ModuleOp moduleOp;
-    if (isa<ModuleOp>(operation)) {
-      moduleOp = llvm::dyn_cast<ModuleOp>(operation);
-    } else {
+    ModuleOp moduleOp = dyn_cast<ModuleOp>(operation);
+    if (!moduleOp)
       moduleOp = operation->getParentOfType<ModuleOp>();
-    }
     return moduleOp.getTargetSystemSpec();
-  } else
-    return TargetSystemSpecInterface();
+  }
+  return TargetSystemSpecInterface();
 }
 
 /// Populates `opsWithLayout` with the list of proper ancestors of `leaf` that
@@ -677,44 +661,24 @@ uint64_t mlir::DataLayout::getStackAlignment() const {
   return *stackAlignment;
 }
 
-std::optional<int64_t> mlir::DataLayout::getMaxVectorOpWidth(
-    TargetSystemSpecInterface::DeviceID deviceID) const {
-  checkValid();
-  DataLayoutEntryInterface entry;
-  if (originalTargetSystemDesc) {
-    if (auto device =
-            originalTargetSystemDesc.getDeviceSpecForDeviceID(deviceID))
-      entry =
-          device->getSpecForIdentifier(device->getMaxVectorOpWidthIdentifier());
-  }
-  // Currently I am not caching the results because we do not return
-  // default values of these properties. Instead if the property is
-  // 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.getMaxVectorOpWidth(entry);
-  else
-    return detail::getMaxVectorOpWidth(entry);
-}
-
-std::optional<int64_t> mlir::DataLayout::getL1CacheSizeInBytes(
-    TargetSystemSpecInterface::DeviceID deviceID) const {
+std::optional<int64_t> mlir::DataLayout::getDevicePropertyValueAsInt(
+    TargetSystemSpecInterface::DeviceID deviceID,
+    StringAttr propertyName) const {
   checkValid();
   DataLayoutEntryInterface entry;
   if (originalTargetSystemDesc) {
-    if (auto device =
+    if (std::optional<TargetDeviceSpecInterface> device =
             originalTargetSystemDesc.getDeviceSpecForDeviceID(deviceID))
-      entry = device->getSpecForIdentifier(
-          device->getL1CacheSizeInBytesIdentifier());
+      entry = device->getSpecForIdentifier(propertyName);
   }
   // Currently I am not caching the results because we do not return
   // default values of these properties. Instead if the property is
   // 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.getL1CacheSizeInBytes(entry);
+    return iface.getDevicePropertyValueAsInt(entry);
   else
-    return detail::getL1CacheSizeInBytes(entry);
+    return detail::getDevicePropertyValueAsInt(entry);
 }
 
 //===----------------------------------------------------------------------===//
@@ -824,47 +788,46 @@ LogicalResult mlir::detail::verifyDataLayoutSpec(DataLayoutSpecInterface spec,
 LogicalResult
 mlir::detail::verifyTargetSystemSpec(TargetSystemSpecInterface spec,
                                      Location loc) {
-  DenseMap<StringAttr, DataLayoutEntryInterface> device_desc_keys;
-  DenseSet<TargetSystemSpecInterface::DeviceID> device_ids;
+  DenseMap<StringAttr, DataLayoutEntryInterface> deviceDescKeys;
+  DenseSet<TargetSystemSpecInterface::DeviceID> deviceIDs;
   for (const auto &entry : spec.getEntries()) {
-    TargetDeviceSpecInterface tdd_spec = entry.second;
+    TargetDeviceSpecInterface targetDeviceSpec = entry.second;
     // First, verify individual target device desc specs.
-    if (failed(tdd_spec.verifyEntry(loc)))
+    if (failed(targetDeviceSpec.verifyEntry(loc)))
       return failure();
 
     // Check that device IDs are unique across all entries.
-    TargetSystemSpecInterface::DeviceID device_id = entry.first;
-    if (!device_ids.insert(device_id).second) {
+    TargetSystemSpecInterface::DeviceID deviceID = entry.first;
+    if (!deviceIDs.insert(deviceID).second) {
       return failure();
     }
 
-    // collect all the keys used by all the tdd_specs.
-    for (DataLayoutEntryInterface entry : tdd_spec.getEntries()) {
+    // collect all the keys used by all the target device specs.
+    for (DataLayoutEntryInterface entry : targetDeviceSpec.getEntries()) {
       if (auto type = llvm::dyn_cast_if_present<Type>(entry.getKey())) {
-        // tdd_spec does not support Type as a key.
+        // targetDeviceSpec does not support Type as a key.
         return failure();
       } else {
-        device_desc_keys[entry.getKey().get<StringAttr>()] = entry;
+        deviceDescKeys[entry.getKey().get<StringAttr>()] = entry;
       }
     }
   }
 
-  for (const auto &kvp : device_desc_keys) {
-    StringAttr identifier = kvp.second.getKey().get<StringAttr>();
-    Dialect *dialect = identifier.getReferencedDialect();
+  for (const auto &[keyName, keyVal] : deviceDescKeys) {
+    Dialect *dialect = keyName.getReferencedDialect();
 
     // Ignore attributes that belong to an unknown dialect, the dialect may
     // actually implement the relevant interface but we don't know about that.
     if (!dialect)
-      continue;
+      return failure();
 
     const auto *iface = dyn_cast<DataLayoutDialectInterface>(dialect);
     if (!iface) {
       return emitError(loc)
              << "the '" << dialect->getNamespace()
              << "' dialect does not support identifier data layout entries";
     }
-    if (failed(iface->verifyEntry(kvp.second, loc)))
+    if (failed(iface->verifyEntry(keyVal, loc)))
       return failure();
   }