Addressing review comments

    Mainly as follows:
    - removing custom attribute parser/printer
    - renaming SystemDescSpec to SystemSpec and TargetDeviceDescSpec to
      TargetDeviceSpec
    - using DeviceID as a type insted of uint32_t
    - grammatical errors

Author: nhasabni

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

@@ -28,7 +28,7 @@ def DataLayoutEntryTrait
 def DLTI_DataLayoutEntryAttr :
     DLTIAttr<"DataLayoutEntry", [DataLayoutEntryTrait]> {
   let summary = [{
-    An attribute to represent an entry of a data layout specification
+    An attribute to represent an entry of a data layout specification.
   }];
   let description = [{
     A data layout entry attribute is a key-value pair where the key is a type or
@@ -38,7 +38,7 @@ def DLTI_DataLayoutEntryAttr :
   let parameters = (ins
     "DataLayoutEntryKey":$key, "Attribute":$value
   );
-  // We do not generate storage class because llvm::PointerUnion
+  // TODO: We do not generate storage class because llvm::PointerUnion
   // does not work with hash_key method.
   let genStorageClass = 0;
   let mnemonic = "dl_entry";
@@ -62,7 +62,9 @@ def DataLayoutSpecTrait
 
 def DLTI_DataLayoutSpecAttr :
     DLTIAttr<"DataLayoutSpec", [DataLayoutSpecTrait]> {
-  let summary = [{An attribute to represent a data layout specification}];
+  let summary = [{
+    An attribute to represent a data layout specification.
+  }];
   let description = [{
     A data layout specification is a list of entries that specify (partial) data
     layout information. It is expected to be attached to operations that serve
@@ -96,84 +98,57 @@ def DLTI_DataLayoutSpecAttr :
     /// Returns the stack alignment identifier.
     StringAttr getStackAlignmentIdentifier(MLIRContext *context) const;
   }];
-  let extraClassDefinition = [{
-    StringAttr
-    $cppClass::getEndiannessIdentifier(MLIRContext *context) const {
-      return Builder(context).getStringAttr(DLTIDialect::kDataLayoutEndiannessKey);
-    }
-
-    StringAttr
-    $cppClass::getAllocaMemorySpaceIdentifier(MLIRContext *context) const {
-      return Builder(context).getStringAttr(
-          DLTIDialect::kDataLayoutAllocaMemorySpaceKey);
-    }
-
-    StringAttr $cppClass::getProgramMemorySpaceIdentifier(
-        MLIRContext *context) const {
-      return Builder(context).getStringAttr(
-          DLTIDialect::kDataLayoutProgramMemorySpaceKey);
-    }
-
-    StringAttr
-    $cppClass::getGlobalMemorySpaceIdentifier(MLIRContext *context) const {
-      return Builder(context).getStringAttr(
-          DLTIDialect::kDataLayoutGlobalMemorySpaceKey);
-    }
-
-    StringAttr
-    $cppClass::getStackAlignmentIdentifier(MLIRContext *context) const {
-      return Builder(context).getStringAttr(
-          DLTIDialect::kDataLayoutStackAlignmentKey);
-    }
-  }];
 }
 
 //===----------------------------------------------------------------------===//
-// TargetSystemDescSpecAttr
+// TargetSystemSpecAttr
 //===----------------------------------------------------------------------===//
 
-def TargetSystemDescSpecTrait
-    : NativeAttrTrait<"TargetSystemDescSpecInterface::Trait"> {
+def TargetSystemSpecTrait
+    : NativeAttrTrait<"TargetSystemSpecInterface::Trait"> {
   let cppNamespace = "::mlir";
 }
 
-def DLTI_TargetSystemDescSpecAttr :
-    DLTIAttr<"TargetSystemDescSpec", [TargetSystemDescSpecTrait]> {
-  let summary = [{An attribute to represent target system description}];
+def DLTI_TargetSystemSpecAttr :
+    DLTIAttr<"TargetSystemSpec", [TargetSystemSpecTrait]> {
+  let summary = [{
+    An attribute to represent target system specification.
+  }];
   let description = [{
-    A system description specification describes the overall system
-    containing multiple devices, with each device having a unique ID
-    and its corresponding TargetDeviceDescSpec object.
+    A system specification describes the overall system containing
+    multiple devices, with each device having a unique ID
+    and its corresponding TargetDeviceSpec object.
 
     Example:
-          dlti.target_system_desc_spec =
-            #dlti.target_device_desc_spec<
+          dlti.target_system_spec =
+            #dlti.target_device_spec<
               #dlti.dl_entry<"dlti.device_id", 0: ui32>,
               #dlti.dl_entry<"dlti.device_type", "CPU">>,
-            #dlti.target_device_desc_spec <
+            #dlti.target_device_spec<
               #dlti.dl_entry<"dlti.device_id", 1: ui32>,
               #dlti.dl_entry<"dlti.device_type", "GPU">,
               #dlti.dl_entry<"dlti.max_vector_op_width", 64 : ui32>>,
-            #dlti.target_device_desc_spec <
+            #dlti.target_device_spec<
               #dlti.dl_entry<"dlti.device_id", 2: ui32>,
               #dlti.dl_entry<"dlti.device_type", "XPU">>>
   }];
   let parameters = (ins
-    ArrayRefParameter<"TargetDeviceDescSpecInterface", "">:$entries
+    ArrayRefParameter<"TargetDeviceSpecInterface", "">:$entries
   );
-  let mnemonic = "target_system_desc_spec";
+  let mnemonic = "target_system_spec";
   let genVerifyDecl = 1;
-  let hasCustomAssemblyFormat = 1;
+  let assemblyFormat = "`<` $entries `>`";
   let extraClassDeclaration = [{
-    /// Return the device descriptor that matches the given device ID
-    std::optional<TargetDeviceDescSpecInterface>
-    getDeviceDescForDeviceID(uint32_t deviceID);
+    /// Return the device specification that matches the given device ID
+    std::optional<TargetDeviceSpecInterface>
+    getDeviceSpecForDeviceID(
+      TargetDeviceSpecInterface::DeviceID deviceID);
   }];
   let extraClassDefinition = [{
-    std::optional<TargetDeviceDescSpecInterface>
-    $cppClass::getDeviceDescForDeviceID(
-        TargetDeviceDescSpecInterface::DeviceID deviceID) {
-      for (TargetDeviceDescSpecInterface entry : getEntries()) {
+    std::optional<TargetDeviceSpecInterface>
+    $cppClass::getDeviceSpecForDeviceID(
+        TargetDeviceSpecInterface::DeviceID deviceID) {
+      for (TargetDeviceSpecInterface entry : getEntries()) {
         if (entry.getDeviceID() == deviceID)
           return entry;
       }
@@ -183,35 +158,37 @@ def DLTI_TargetSystemDescSpecAttr :
 }
 
 //===----------------------------------------------------------------------===//
-// TargetDeviceDescSpecAttr
+// TargetDeviceSpecAttr
 //===----------------------------------------------------------------------===//
 
-def TargetDeviceDescSpecTrait
-    : NativeAttrTrait<"TargetDeviceDescSpecInterface::Trait"> {
+def TargetDeviceSpecTrait
+    : NativeAttrTrait<"TargetDeviceSpecInterface::Trait"> {
   let cppNamespace = "::mlir";
 }
 
-def DLTI_TargetDeviceDescSpecAttr :
-    DLTIAttr<"TargetDeviceDescSpec", [TargetDeviceDescSpecTrait]> {
-  let summary = [{An attribute to represent target device description}];
+def DLTI_TargetDeviceSpecAttr :
+    DLTIAttr<"TargetDeviceSpec", [TargetDeviceSpecTrait]> {
+  let summary = [{
+    An attribute to represent target device specification.
+  }];
   let description = [{
-        Each device description specification describes a single device and
-        its hardware properties. Each device description must have a device_id
-        and a device_type. In addition, the description can contain any number
-        of optional hardware properties (e.g., max_vector_op_width below).
+    Each device specification describes a single device and its
+    hardware properties. Each device specification must have a device_id
+    and a device_type. In addition, the specification can contain any number
+    of optional hardware properties (e.g., max_vector_op_width below).
 
-        Example:
-          #dlti.target_device_desc_spec <
-                #dlti.dl_entry<"dlti.device_id", 1: ui32>,
-                #dlti.dl_entry<"dlti.device_type", "GPU">,
-                #dlti.dl_entry<"dlti.max_vector_op_width", 64 : ui32>>
-        }];
+    Example:
+      #dlti.target_device_spec<
+            #dlti.dl_entry<"dlti.device_id", 1: ui32>,
+            #dlti.dl_entry<"dlti.device_type", "GPU">,
+            #dlti.dl_entry<"dlti.max_vector_op_width", 64 : ui32>>
+  }];
   let parameters = (ins
     ArrayRefParameter<"DataLayoutEntryInterface", "">:$entries
   );
-  let mnemonic = "target_device_desc_spec";
+  let mnemonic = "target_device_spec";
   let genVerifyDecl = 1;
-  let hasCustomAssemblyFormat = 1;
+  let assemblyFormat = "`<` $entries `>`";
   let extraClassDeclaration = [{
     /// Returns the device ID identifier.
     StringAttr getDeviceIDIdentifier();
@@ -248,55 +225,7 @@ def DLTI_TargetDeviceDescSpecAttr :
     DataLayoutEntryInterface getSpecForL1CacheSizeInBytes();
 
     /// Return the value of device ID
-    uint32_t getDeviceID();
-  }];
-
-  let extraClassDefinition = [{
-    StringAttr
-    $cppClass::getDeviceIDIdentifier() {
-      return Builder(getContext()).getStringAttr(DLTIDialect::kTargetDeviceIDKey);
-    }
-
-    StringAttr
-    $cppClass::getDeviceTypeIdentifier() {
-      return Builder(getContext()).getStringAttr(DLTIDialect::kTargetDeviceTypeKey);
-    }
-
-    StringAttr
-    $cppClass::getMaxVectorOpWidthIdentifier() {
-      return Builder(getContext()).getStringAttr(
-          DLTIDialect::kTargetDeviceMaxVectorOpWidthKey);
-    }
-
-    StringAttr $cppClass::getL1CacheSizeInBytesIdentifier() {
-      return Builder(getContext()).getStringAttr(
-          DLTIDialect::kTargetDeviceL1CacheSizeInBytesKey);
-    }
-
-    DataLayoutEntryInterface
-    $cppClass::getSpecForDeviceID() {
-      return getSpecForIdentifier(getDeviceIDIdentifier());
-    }
-
-    DataLayoutEntryInterface
-    $cppClass::getSpecForDeviceType() {
-      return getSpecForIdentifier(getDeviceTypeIdentifier());
-    }
-
-    DataLayoutEntryInterface
-    $cppClass::getSpecForMaxVectorOpWidth() {
-      return getSpecForIdentifier(getMaxVectorOpWidthIdentifier());
-    }
-
-    DataLayoutEntryInterface
-    $cppClass::getSpecForL1CacheSizeInBytes() {
-      return getSpecForIdentifier(getL1CacheSizeInBytesIdentifier());
-    }
-
-    uint32_t $cppClass::getDeviceID() {
-      DataLayoutEntryInterface entry = getSpecForDeviceID();
-      return llvm::cast<IntegerAttr>(entry.getValue()).getValue().getZExtValue();
-    }
+    TargetDeviceSpecInterface::DeviceID getDeviceID();
   }];
 }
 

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

@@ -29,10 +29,10 @@ def DLTI_Dialect : Dialect {
 
     // Top level attribute name for target system description
     constexpr const static ::llvm::StringLiteral
-    kTargetSystemDescAttrName = "dlti.target_system_desc_spec";
+    kTargetSystemDescAttrName = "dlti.target_system_spec";
 
     constexpr const static ::llvm::StringLiteral
-    kTargetDeviceDescAttrName = "dlti.target_device_desc_spec";
+    kTargetDeviceDescAttrName = "dlti.target_device_spec";
 
     // Constants used in entries.
     constexpr const static ::llvm::StringLiteral
@@ -56,7 +56,7 @@ def DLTI_Dialect : Dialect {
     constexpr const static ::llvm::StringLiteral
     kDataLayoutStackAlignmentKey = "dlti.stack_alignment";
 
-    // Constants used in target description part of DLTI
+    // Constants used in target description part of DLTI.
     constexpr const static ::llvm::StringLiteral
     kTargetDeviceIDKey = "dlti.device_id";
 

mlir/include/mlir/Dialect/DLTI/Traits.h

@@ -18,7 +18,7 @@ class DataLayoutSpecAttr;
 namespace impl {
 LogicalResult verifyHasDefaultDLTIDataLayoutTrait(Operation *op);
 DataLayoutSpecInterface getDataLayoutSpec(Operation *op);
-TargetSystemDescSpecInterface getTargetSystemDescSpec(Operation *op);
+TargetSystemSpecInterface getTargetSystemSpec(Operation *op);
 } // namespace impl
 
 /// Trait to be used by operations willing to use the implementation of the
@@ -41,8 +41,8 @@ class HasDefaultDLTIDataLayout
 
   /// Returns the target system description specification as provided by DLTI
   /// dialect
-  TargetSystemDescSpecInterface getTargetSystemDescSpec() {
-    return impl::getTargetSystemDescSpec(this->getOperation());
+  TargetSystemSpecInterface getTargetSystemSpec() {
+    return impl::getTargetSystemSpec(this->getOperation());
   }
 };
 } // namespace mlir

mlir/include/mlir/IR/BuiltinOps.td

@@ -78,7 +78,7 @@ def ModuleOp : Builtin_Op<"module", [
     //===------------------------------------------------------------------===//
 
     DataLayoutSpecInterface getDataLayoutSpec();
-    TargetSystemDescSpecInterface getTargetSystemDescSpec();
+    TargetSystemSpecInterface getTargetSystemSpec();
 
     //===------------------------------------------------------------------===//
     // OpAsmOpInterface Methods

mlir/include/mlir/Interfaces/DataLayoutInterfaces.h

@@ -23,15 +23,14 @@
 namespace mlir {
 class DataLayout;
 class DataLayoutEntryInterface;
-class TargetDeviceDescSpecInterface;
-class TargetSystemDescSpecInterface;
+class TargetDeviceSpecInterface;
+class TargetSystemSpecInterface;
 using DataLayoutEntryKey = llvm::PointerUnion<Type, StringAttr>;
 // Using explicit SmallVector size because we cannot infer the size from the
 // forward declaration, and we need the typedef in the actual declaration.
 using DataLayoutEntryList = llvm::SmallVector<DataLayoutEntryInterface, 4>;
 using DataLayoutEntryListRef = llvm::ArrayRef<DataLayoutEntryInterface>;
-using TargetDeviceDescSpecListRef =
-    llvm::ArrayRef<TargetDeviceDescSpecInterface>;
+using TargetDeviceSpecListRef = llvm::ArrayRef<TargetDeviceSpecInterface>;
 class DataLayoutOpInterface;
 class DataLayoutSpecInterface;
 class ModuleOp;
@@ -109,8 +108,8 @@ filterEntryForIdentifier(DataLayoutEntryListRef entries, StringAttr id);
 
 /// Given a list of target device entries, returns the entry that has the given
 /// identifier as key, if such an entry exists in the list.
-TargetDeviceDescSpecInterface
-filterEntryForIdentifier(TargetDeviceDescSpecListRef entries, StringAttr id);
+TargetDeviceSpecInterface
+filterEntryForIdentifier(TargetDeviceSpecListRef entries, StringAttr id);
 
 /// Verifies that the operation implementing the data layout interface, or a
 /// module operation, is valid. This calls the verifier of the spec attribute
@@ -126,8 +125,8 @@ LogicalResult verifyDataLayoutSpec(DataLayoutSpecInterface spec, Location loc);
 /// Verifies that a target system desc spec is valid. This dispatches to
 /// individual entry verifiers, and then to the verifiers implemented by the
 /// relevant dialect interfaces for identifier keys.
-LogicalResult verifyTargetSystemDescSpec(TargetSystemDescSpecInterface spec,
-                                         Location loc);
+LogicalResult verifyTargetSystemSpec(TargetSystemSpecInterface spec,
+                                     Location loc);
 
 /// Divides the known min value of the numerator by the denominator and rounds
 /// the result up to the next integer. Preserves the scalable flag.
@@ -162,7 +161,7 @@ class DataLayoutDialectInterface
 
   /// Checks whether the given data layout entry is valid and reports any errors
   /// at the provided location. Derived classes should override this.
-  virtual LogicalResult verifyEntry(TargetDeviceDescSpecInterface entry,
+  virtual LogicalResult verifyEntry(TargetDeviceSpecInterface entry,
                                     Location loc) const {
     return success();
   }
@@ -247,19 +246,19 @@ class DataLayout {
   /// Returns for max vector op width if the property is defined for the given
   /// device ID, otherwise return std::nullopt.
   std::optional<uint32_t>
-      getMaxVectorOpWidth(TargetDeviceDescSpecInterface::DeviceID) const;
+      getMaxVectorOpWidth(TargetDeviceSpecInterface::DeviceID) const;
 
   /// Returns for L1 cache size if the property is defined for the given
   /// device ID, otherwise return std::nullopt.
   std::optional<uint32_t>
-      getL1CacheSizeInBytes(TargetDeviceDescSpecInterface::DeviceID) const;
+      getL1CacheSizeInBytes(TargetDeviceSpecInterface::DeviceID) const;
 
 private:
   /// Combined layout spec at the given scope.
   const DataLayoutSpecInterface originalLayout;
 
   /// Combined target system desc spec at the given scope.
-  const TargetSystemDescSpecInterface originalTargetSystemDesc;
+  const TargetSystemSpecInterface originalTargetSystemDesc;
 
 #if LLVM_ENABLE_ABI_BREAKING_CHECKS
   /// List of enclosing layout specs.