Address review comments.

mshahneo · mshahneo · commit f33b7f73840a · 2025-10-08T13:28:10.000Z
Simplify the design:
- Remove uArchHierarchyComponent

LLVMize names.
Replace String usage with enum whenever possible.
diff --git a/mlir/include/mlir/Dialect/XeGPU/uArch/IntelGpuXe2.h b/mlir/include/mlir/Dialect/XeGPU/uArch/IntelGpuXe2.h
@@ -45,11 +45,10 @@ struct Xe2Plus : public uArch {
   Xe2Plus(
       const std::string &archName, const std::string &archDescription,
       const XeCoreInfo &xeCore,
-      const std::vector<uArchHierarchyComponent> &hierarchy = {},
-      const std::map<std::string, RegisterFileInfo> &regInfo = {},
+      const std::map<RegisterFileType, RegisterFileInfo> &regInfo = {},
       const std::vector<CacheInfo> &cacheInfo = {},
       const std::map<std::string, std::shared_ptr<Instruction>> &instrs = {})
-      : uArch(archName, archDescription, hierarchy, regInfo, cacheInfo, instrs),
+      : uArch(archName, archDescription, regInfo, cacheInfo, instrs),
         xe_core(xeCore) {}
 };
 
@@ -62,9 +61,9 @@ struct DPASInstruction : public Instruction, public MMAInstructionInterface {
 
   // Override all virtuals from MatrixOpInterface
   virtual std::vector<std::pair<uint32_t, uint32_t>>
-  getSupportedShapes(mlir::Type dataType, MMAOpndEnum matrixType) override;
+  getSupportedShapes(mlir::Type dataType, MMAOpndKind matrixType) override;
   virtual std::vector<mlir::Type>
-  getSupportedTypes(MLIRContext &context, MMAOpndEnum matrixType) override;
+  getSupportedTypes(MLIRContext &context, MMAOpndKind matrixType) override;
   virtual bool
   checkSupportedShapesAndTypes(std::pair<uint32_t, uint32_t> AShape,
                                std::pair<uint32_t, uint32_t> BShape,
@@ -97,29 +96,22 @@ struct PVCuArch : public Xe2Plus {
                 {/* cache_info */},         // Optional: empty
                 {/* instructions */}        // Optional: empty
         ) {
-    // Initialize uArchHierarchy
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("thread", 0));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("XeCore", 8));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("XeSlice", 16));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("XeStack", 4));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("gpu", 2));
     // Intialize register file info
     // GRF
-    this->register_file_info.emplace(
-        "GRF",
-        RegisterFileInfo(64 * 1024,          // size in bits
-                         {"small", "large"}, // GRF modes
-                         {128, 256},         // registers per thread per mode
-                         0,                  // number of banks
-                         0                   // bank size
-                         ));
+    this->registerFileInfo.emplace(
+        RegisterFileType::GRF,
+        RegisterFileInfo(
+            64 * 1024,                                          // size in bits
+            {RegisterFileMode::Small, RegisterFileMode::Large}, // GRF modes
+            {128, 256} // registers per thread per mode
+            ));
     // Initialize cache info
     // L1 cache, XeCore level
-    this->cache_info.push_back(
-        CacheInfo(512 * 1024, 64, this->uArch_hierarchy[1]));
-    // L3 cache, XeStack level
-    this->cache_info.push_back(
-        CacheInfo(512 * 1024, 64, this->uArch_hierarchy[3]));
+    this->cacheInfo.push_back(
+        CacheInfo(512 * 1024, 64, CacheHierarchyLevel::L1));
+    // L2 cache, XeStack level
+    this->cacheInfo.push_back(
+        CacheInfo(512 * 1024, 64, CacheHierarchyLevel::L2));
 
     // Add the instructions
     auto dpas = std::make_shared<DPASInstruction>();
@@ -140,31 +132,22 @@ struct BMGuArch : public Xe2Plus {
                 XeCoreInfo(8, SharedMemory(256 * 1024, 4), 8, 8), // xeCore
                 {/* register_file_info */}, // Optional: empty
                 {/* cache_info */},         // Optional: empty
-                {/* instructions */},       // Optional: empty
-                {/* restrictions */}        // Optional: empty
+                {/* instructions */}        // Optional: empty)
         ) {
-    // Initialize uArchHierarchy
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("thread", 0));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("XeCore", 8));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("XeSlice", 4));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("XeStack", 5));
-    this->uArch_hierarchy.push_back(uArchHierarchyComponent("gpu", 1));
     // Intialize register file info
     // GRF
-    this->register_file_info["GRF"] =
-        RegisterFileInfo(64 * 1024,          // size in bits
-                         {"small", "large"}, // GRF modes
-                         {128, 256},         // registers per thread per mode
-                         0,                  // number of banks
-                         0                   // bank size
-        );
+    this->registerFileInfo[RegisterFileType::GRF] = RegisterFileInfo(
+        64 * 1024,                                          // size in bits
+        {RegisterFileMode::Small, RegisterFileMode::Large}, // GRF modes
+        {128, 256} // registers per thread per mode
+    );
     // Initialize cache info
     // L1 cache, XeCore level
-    this->cache_info.push_back(
-        CacheInfo(256 * 1024, 64, this->uArch_hierarchy[1]));
-    // L3 cache, XeStack level
-    this->cache_info.push_back(
-        CacheInfo(18 * 1024 * 1024, 256, this->uArch_hierarchy[3]));
+    this->cacheInfo.push_back(
+        CacheInfo(256 * 1024, 64, CacheHierarchyLevel::L1));
+    // L2 cache, XeStack level
+    this->cacheInfo.push_back(
+        CacheInfo(18 * 1024 * 1024, 256, CacheHierarchyLevel::L2));
 
     // Add the instructions
     auto dpas = std::make_shared<DPASInstruction>();
diff --git a/mlir/include/mlir/Dialect/XeGPU/uArch/uArchBase.h b/mlir/include/mlir/Dialect/XeGPU/uArch/uArchBase.h
@@ -27,19 +27,15 @@
 namespace mlir {
 namespace xegpu {
 namespace uArch {
-// Architecture HW component hierarchy to present thread, core, socket ...
-struct uArchHierarchyComponent {
-  std::string name = ""; // optional name of the hierarchy component
-  // no. of lower hierarchy component it contains, e.g., for PVC XeCore it
-  // contains 8 threads, so no_of_component=8
-  uint32_t no_of_component;
-  // Constructor
-  uArchHierarchyComponent(const std::string &name, uint32_t no_of_component)
-      : name(name), no_of_component(no_of_component) {}
-};
 
 // An enum class to represent the scope of an instruction
-enum class InstructionScopeEnum { WorkItem, Subgroup, Workgroup, Cluster };
+enum class InstructionScope { WorkItem, Subgroup, Workgroup, Cluster };
+
+enum class InstructionName {
+  DPAS, // Dot Product Accumulate Systolic (DPAS) is a matrix multiply-add
+        // operation
+  // Add more instructions as needed
+};
 
 // A struct to represent basic information about an instruction
 // This struct is used to represent the information about an instruction in the
@@ -67,69 +63,62 @@ struct Instruction {
   // Get methods
   std::string getName() { return name; }
   std::string getDescription() { return description; }
-  InstructionScopeEnum getScope() { return scope; }
+  InstructionScope getScope() { return scope; }
 
 protected:
   std::string name;
   std::string description;
-  InstructionScopeEnum scope;
+  InstructionScope scope;
 };
 
+enum class RegisterFileMode : uint8_t { Small, Large };
+enum class RegisterFileType : uint8_t { GRF, ARF };
+
 // A struct to represent register file information
 struct RegisterFileInfo {
   // Constructor
   RegisterFileInfo() = default;
-  RegisterFileInfo(uint32_t size, const std::vector<std::string> &mode,
-                   const std::vector<uint32_t> &numRegs, uint32_t num_banks,
-                   uint32_t bank_size)
-      : size(size), mode(mode), num_regs_per_thread_per_mode(numRegs),
-        num_banks(num_banks), bank_size(bank_size) {}
+  RegisterFileInfo(uint32_t size, const std::vector<RegisterFileMode> &mode,
+                   const std::vector<uint32_t> &numRegs)
+      : size(size), mode(mode), numRegsPerThreadPerMode(numRegs) {}
 
-  // Get methods
   uint32_t getSize() const { return size; }
-
-  const std::vector<std::string> &getModes() const { return mode; }
-
+  const std::vector<RegisterFileMode> &getModes() const { return mode; }
   const std::vector<uint32_t> &getNumRegsPerThreadPerMode() const {
-    return num_regs_per_thread_per_mode;
+    return numRegsPerThreadPerMode;
   }
 
-  uint32_t getNumBanks() const { return num_banks; }
-
-  uint32_t getBankSize() const { return bank_size; }
-
 protected:
-  uint32_t size;                 // size per register in bits
-  std::vector<std::string> mode; // e.g., "small", "large" GRF modes
+  uint32_t size;                      // size per register in bits
+  std::vector<RegisterFileMode> mode; // e.g., "small", "large" GRF modes
   std::vector<uint32_t>
-      num_regs_per_thread_per_mode; // number of registers per thread per mode
-  uint32_t num_banks;
-  uint32_t bank_size;
+      numRegsPerThreadPerMode; // number of registers per thread per mode
+  // TODO: Add more fields as needed (e.g., num_banks, bank_size, num_ports,
+  // port_width, bank_conflicts)
 };
 
+enum class CacheHierarchyLevel { L1 = 1, L2 = 2, L3 = 3 };
 // A struct to represent cache information
-
 struct CacheInfo {
   // Constructor
   CacheInfo(uint32_t size, uint32_t line_size,
-            const uArchHierarchyComponent &component)
-      : size(size), line_size(line_size), component(component) {}
+            CacheHierarchyLevel hierarchy_level)
+      : size(size), line_size(line_size), hierarchy_level(hierarchy_level) {}
 
   virtual ~CacheInfo() = default;
 
   // Get methods
   uint32_t getSize() const { return size; }
   uint32_t getLineSize() const { return line_size; }
-  const uArchHierarchyComponent &getComponent() const { return component; }
+  CacheHierarchyLevel getHierarchyLevel() const { return hierarchy_level; }
 
 protected:
   uint32_t size;
   uint32_t line_size;
-  // At which component level the cache is shared
-  uArchHierarchyComponent component;
-
+  CacheHierarchyLevel hierarchy_level;
   // @TODO: Add more fields as needed (e.g., associativity, num_banks,
-  // bank_size, num_ports, port_width, bank_conflicts)
+  // bank_size, num_ports, port_width, bank_conflicts, hierarchy_level,
+  // latency, throughput, bandwidth)
 };
 
 // A struct to represent the uArch
@@ -145,29 +134,26 @@ struct uArch {
   // Constructor
   uArch() = default;
   uArch(const std::string &name, const std::string &description,
-        const std::vector<uArchHierarchyComponent> &uArch_hierarchy = {},
-        const std::map<std::string, RegisterFileInfo> &register_file_info = {},
+        const std::map<RegisterFileType, RegisterFileInfo> &register_file_info =
+            {},
         const std::vector<CacheInfo> &cache_info = {},
         const std::map<std::string, std::shared_ptr<Instruction>>
             &instructions = {})
-      : name(name), description(description), uArch_hierarchy(uArch_hierarchy),
-        register_file_info(register_file_info), cache_info(cache_info),
+      : name(name), description(description),
+        registerFileInfo(register_file_info), cacheInfo(cache_info),
         instructions(instructions) {}
 
   // Get methods
   const std::string &getName() const { return name; }
 
   const std::string &getDescription() const { return description; }
 
-  const std::vector<uArchHierarchyComponent> &getHierarchy() const {
-    return uArch_hierarchy;
-  }
-
-  const std::map<std::string, RegisterFileInfo> &getRegisterFileInfo() const {
-    return register_file_info;
+  const std::map<RegisterFileType, RegisterFileInfo> &
+  getRegisterFileInfo() const {
+    return registerFileInfo;
   }
 
-  const std::vector<CacheInfo> &getCacheInfo() const { return cache_info; }
+  const std::vector<CacheInfo> &getCacheInfo() const { return cacheInfo; }
 
   const std::map<std::string, std::shared_ptr<Instruction>> &
   getInstructions() const {
@@ -192,9 +178,8 @@ struct uArch {
 protected:
   std::string name; // Similar to target triple
   std::string description;
-  std::vector<uArchHierarchyComponent> uArch_hierarchy;
-  std::map<std::string, RegisterFileInfo> register_file_info;
-  std::vector<CacheInfo> cache_info;
+  std::map<RegisterFileType, RegisterFileInfo> registerFileInfo;
+  std::vector<CacheInfo> cacheInfo;
   std::map<std::string, std::shared_ptr<Instruction>> instructions;
 };
 
diff --git a/mlir/include/mlir/Dialect/XeGPU/uArch/uArchInterfaces.h b/mlir/include/mlir/Dialect/XeGPU/uArch/uArchInterfaces.h
@@ -26,12 +26,11 @@ namespace mlir {
 namespace xegpu {
 namespace uArch {
 
-enum class MMAOpndEnum { MatrixA, MatrixB, MatrixC, MatrixD };
+enum class MMAOpndKind { MatrixA, MatrixB, MatrixC, MatrixD };
 struct MMAInstructionInterface {
   // Get supported Matrix shapes
   virtual std::vector<std::pair<uint32_t, uint32_t>>
-  getSupportedShapes(mlir::Type dataType, MMAOpndEnum matrixType) = 0;
-
+  getSupportedShapes(mlir::Type dataType, MMAOpndKind matrixType) = 0;
   // @TODO: This method takes an context object as a parameter, this is to
   // create the mlir::Type objects from the same context. Since type objects are
   // uniqued in a specific context, to do things like "aType == bType" (where
@@ -46,7 +45,7 @@ struct MMAInstructionInterface {
   // Untill we have a better solution, we stick to passing context object to
   // this method.
   virtual std::vector<mlir::Type> getSupportedTypes(MLIRContext &context,
-                                                    MMAOpndEnum matrixType) = 0;
+                                                    MMAOpndKind matrixType) = 0;
   virtual bool
   checkSupportedShapesAndTypes(std::pair<uint32_t, uint32_t> AShape,
                                std::pair<uint32_t, uint32_t> BShape,
diff --git a/mlir/lib/Dialect/XeGPU/uArch/IntelGpuXe2.cpp b/mlir/lib/Dialect/XeGPU/uArch/IntelGpuXe2.cpp
@@ -16,7 +16,7 @@ namespace Xe2Plus {
 
 std::vector<std::pair<uint32_t, uint32_t>>
 DPASInstruction::getSupportedShapes(mlir::Type dataType,
-                                    MMAOpndEnum matrixType) {
+                                    MMAOpndKind matrixType) {
   auto combineVectors = [](const std::vector<uint32_t> &a,
                            const std::vector<uint32_t> &b)
       -> std::vector<std::pair<uint32_t, uint32_t>> {
@@ -35,16 +35,16 @@ DPASInstruction::getSupportedShapes(mlir::Type dataType,
   std::vector<std::pair<unsigned, unsigned>> resultMatrix;
 
   switch (matrixType) {
-  case MMAOpndEnum::MatrixA:
+  case MMAOpndKind::MatrixA:
     resultMatrix = combineVectors(M, K);
     break;
-  case MMAOpndEnum::MatrixB:
+  case MMAOpndKind::MatrixB:
     resultMatrix = combineVectors(K, N);
     break;
-  case MMAOpndEnum::MatrixC:
+  case MMAOpndKind::MatrixC:
     resultMatrix = combineVectors(M, N);
     break;
-  case MMAOpndEnum::MatrixD:
+  case MMAOpndKind::MatrixD:
     resultMatrix = combineVectors(M, N);
     break;
   }
@@ -53,23 +53,23 @@ DPASInstruction::getSupportedShapes(mlir::Type dataType,
 
 std::vector<mlir::Type>
 DPASInstruction::getSupportedTypes(MLIRContext &context,
-                                   MMAOpndEnum matrixType) {
+                                   MMAOpndKind matrixType) {
   mlir::Type bf16Type = mlir::BFloat16Type::get(&context);
   mlir::Type f16Type = mlir::Float16Type::get(&context);
   mlir::Type tf32Type = mlir::FloatTF32Type::get(&context);
   mlir::Type f32Type = mlir::Float32Type::get(&context);
 
   switch (matrixType) {
-  case MMAOpndEnum::MatrixA:
+  case MMAOpndKind::MatrixA:
     return {bf16Type, f16Type, tf32Type};
     break;
-  case MMAOpndEnum::MatrixB:
+  case MMAOpndKind::MatrixB:
     return {bf16Type, f16Type, tf32Type};
     break;
-  case MMAOpndEnum::MatrixC:
+  case MMAOpndKind::MatrixC:
     return {bf16Type, f16Type, f32Type};
     break;
-  case MMAOpndEnum::MatrixD:
+  case MMAOpndKind::MatrixD:
     return {bf16Type, f16Type, f32Type};
     break;
   }
@@ -110,10 +110,10 @@ bool DPASInstruction::checkSupportedShapesAndTypes(
     std::pair<uint32_t, uint32_t> AShape, std::pair<uint32_t, uint32_t> BShape,
     std::pair<uint32_t, uint32_t> CShape, std::pair<uint32_t, uint32_t> DShape,
     mlir::Type AType, mlir::Type BType, mlir::Type CType, mlir::Type DType) {
-  auto supportedAShapes = getSupportedShapes(AType, MMAOpndEnum::MatrixA);
-  auto supportedBShapes = getSupportedShapes(BType, MMAOpndEnum::MatrixB);
-  auto supportedCShapes = getSupportedShapes(CType, MMAOpndEnum::MatrixC);
-  auto supportedDShapes = getSupportedShapes(DType, MMAOpndEnum::MatrixD);
+  auto supportedAShapes = getSupportedShapes(AType, MMAOpndKind::MatrixA);
+  auto supportedBShapes = getSupportedShapes(BType, MMAOpndKind::MatrixB);
+  auto supportedCShapes = getSupportedShapes(CType, MMAOpndKind::MatrixC);
+  auto supportedDShapes = getSupportedShapes(DType, MMAOpndKind::MatrixD);
   return llvm::is_contained(supportedAShapes, AShape) &&
          llvm::is_contained(supportedBShapes, BShape) &&
          llvm::is_contained(supportedCShapes, CShape) &&
