[OM] Separates OM object fields verifier to a dedicated pass

include/circt/Dialect/OM/OMOps.td

@@ -149,8 +149,7 @@ def ObjectOp : OMOp<"object",
   }];
 }
 
-def ObjectFieldOp : OMOp<"object.field",
-    [DeclareOpInterfaceMethods<SymbolUserOpInterface>, Pure]> {
+def ObjectFieldOp : OMOp<"object.field", [Pure]> {
   let arguments = (ins
     ClassType:$object,
     FlatSymbolRefArrayAttr:$fieldPath

include/circt/Dialect/OM/OMPasses.h

@@ -21,6 +21,7 @@ namespace om {
 
 std::unique_ptr<mlir::Pass> createOMLinkModulesPass();
 std::unique_ptr<mlir::Pass> createFreezePathsPass();
+std::unique_ptr<mlir::Pass> createVerifyObjectFieldsPass();
 
 #define GEN_PASS_REGISTRATION
 #include "circt/Dialect/OM/OMPasses.h.inc"

include/circt/Dialect/OM/OMPasses.td

@@ -25,3 +25,11 @@ def LinkModules: Pass<"om-link-modules", "mlir::ModuleOp"> {
   }];
   let constructor = "circt::om::createOMLinkModulesPass()";
 }
+
+def VerifyObjectFields: Pass<"om-verify-object-fields"> {
+  let summary = "Verify fields of ObjectOp are valid";
+  let description = [{
+    Verify object fields are valid.
+  }];
+  let constructor = "circt::om::createVerifyObjectFieldsPass()";
+}

lib/Dialect/OM/OMOps.cpp

@@ -330,69 +330,6 @@ circt::om::ObjectOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
   return success();
 }
 
-//===----------------------------------------------------------------------===//
-// ObjectFieldOp
-//===----------------------------------------------------------------------===//
-
-LogicalResult
-circt::om::ObjectFieldOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
-  // Get the ObjectInstOp and the ClassLike it is an instance of.
-  ObjectOp objectInst = getObject().getDefiningOp<ObjectOp>();
-  ClassLike classDef = cast<ClassLike>(symbolTable.lookupNearestSymbolFrom(
-      *this, objectInst.getClassNameAttr()));
-
-  // Traverse the field path, verifying each field exists.
-  ClassFieldLike finalField;
-  auto fields = SmallVector<FlatSymbolRefAttr>(
-      getFieldPath().getAsRange<FlatSymbolRefAttr>());
-  for (size_t i = 0, e = fields.size(); i < e; ++i) {
-    // Verify the field exists on the ClassOp.
-    auto field = fields[i];
-    ClassFieldLike fieldDef;
-    classDef.walk([&](ClassFieldLike fieldLike) {
-      if (fieldLike.getNameAttr() == field.getAttr()) {
-        fieldDef = fieldLike;
-        return WalkResult::interrupt();
-      }
-      return WalkResult::advance();
-    });
-    if (!fieldDef) {
-      auto error = emitOpError("referenced non-existant field ") << field;
-      error.attachNote(classDef.getLoc()) << "class defined here";
-      return error;
-    }
-
-    // If there are more fields, verify the current field is of ClassType, and
-    // look up the ClassOp for that field.
-    if (i < e - 1) {
-      auto classType = dyn_cast<ClassType>(fieldDef.getType());
-      if (!classType)
-        return emitOpError("nested field access into ")
-               << field << " requires a ClassType, but found "
-               << fieldDef.getType();
-
-      // The nested ClassOp must exist, since a field with ClassType must be
-      // an ObjectInstOp, which already verifies the class exists.
-      classDef = cast<ClassLike>(
-          symbolTable.lookupNearestSymbolFrom(*this, classType.getClassName()));
-
-      // Proceed to the next field in the path.
-      continue;
-    }
-
-    // On the last iteration down the path, save the final field being accessed.
-    finalField = fieldDef;
-  }
-
-  // Verify the accessed field type matches the result type.
-  if (finalField.getType() != getResult().getType())
-    return emitOpError("expected type ")
-           << getResult().getType() << ", but accessed field has type "
-           << finalField.getType();
-
-  return success();
-}
-
 //===----------------------------------------------------------------------===//
 // ConstantOp
 //===----------------------------------------------------------------------===//

lib/Dialect/OM/Transforms/CMakeLists.txt

@@ -1,6 +1,7 @@
 add_circt_dialect_library(CIRCTOMTransforms
   FreezePaths.cpp
   LinkModules.cpp
+  VerifyObjectFields.cpp
 
   DEPENDS
   CIRCTOMTransformsIncGen

lib/Dialect/OM/Transforms/VerifyObjectFields.cpp

@@ -0,0 +1,161 @@
+//===- VerifyObjectFields.cpp - Verify Object fields -------------*- C++-*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Contains the definitions of the OM verify object fields pass.
+//
+//===----------------------------------------------------------------------===//
+
+#include "PassDetails.h"
+#include "circt/Dialect/HW/HWInstanceGraph.h"
+#include "circt/Dialect/HW/HWOps.h"
+#include "circt/Dialect/HW/InnerSymbolTable.h"
+#include "circt/Dialect/OM/OMAttributes.h"
+#include "circt/Dialect/OM/OMOps.h"
+#include "circt/Dialect/OM/OMPasses.h"
+#include "mlir/IR/Threading.h"
+#include "llvm/ADT/DenseMap.h"
+
+using namespace mlir;
+using namespace circt;
+using namespace om;
+
+namespace {
+struct VerifyObjectFieldsPass
+    : public VerifyObjectFieldsBase<VerifyObjectFieldsPass> {
+  void runOnOperation() override;
+  bool canScheduleOn(RegisteredOperationName opName) const override {
+    return opName.getStringRef() == "firrtl.circuit" ||
+           opName.getStringRef() == "builtin.module";
+  }
+};
+} // namespace
+
+void VerifyObjectFieldsPass::runOnOperation() {
+  auto *module = getOperation();
+  assert(module->getNumRegions() == 1 &&
+         module->hasTrait<OpTrait::SymbolTable>() &&
+         "op must have a single region and symbol table trait");
+  auto &symbolTable = getAnalysis<SymbolTable>();
+
+  /// A map from a class and field name to a field.
+  llvm::MapVector<ClassLike, llvm::DenseMap<StringAttr, ClassFieldLike>> tables;
+  for (auto op : module->getRegion(0).getOps<om::ClassLike>())
+    tables.insert({op, llvm::DenseMap<StringAttr, ClassFieldLike>()});
+
+  // Peel tables parallelly.
+  if (failed(
+          mlir::failableParallelForEach(&getContext(), tables, [](auto &entry) {
+            ClassLike classLike = entry.first;
+            auto &table = entry.second;
+            auto result = classLike.walk([&](ClassFieldLike fieldLike)
+                                             -> WalkResult {
+              if (table.insert({fieldLike.getNameAttr(), fieldLike}).second)
+                return WalkResult::advance();
+
+              auto emit = fieldLike.emitOpError()
+                          << "field " << fieldLike.getNameAttr()
+                          << " is defined twice";
+              emit.attachNote(table.lookup(fieldLike.getNameAttr()).getLoc())
+                  << "previous definition is here";
+              return WalkResult::interrupt();
+            });
+            return LogicalResult::failure(result.wasInterrupted());
+          })))
+    return signalPassFailure();
+
+  // Run actual verification. Make sure not to mutate `tables`.
+  auto result = mlir::failableParallelForEach(
+      &getContext(), tables, [&tables, &symbolTable](const auto &entry) {
+        ClassLike classLike = entry.first;
+        auto result =
+            classLike.walk([&](ObjectFieldOp objectField) -> WalkResult {
+              auto objectInstType =
+                  cast<ClassType>(objectField.getObject().getType());
+              ClassLike classDef =
+                  symbolTable.lookupNearestSymbolFrom<ClassLike>(
+                      objectField, objectInstType.getClassName());
+              if (!classDef) {
+                objectField.emitError()
+                    << "class " << objectInstType.getClassName()
+                    << " was not found";
+                return WalkResult::interrupt();
+              }
+
+              // Traverse the field path, verifying each field exists.
+              ClassFieldLike finalField;
+              auto fields = SmallVector<FlatSymbolRefAttr>(
+                  objectField.getFieldPath().getAsRange<FlatSymbolRefAttr>());
+              for (size_t i = 0, e = fields.size(); i < e; ++i) {
+                // Verify the field exists on the ClassOp.
+                auto field = fields[i];
+                ClassFieldLike fieldDef;
+                auto *it = tables.find(classDef);
+                assert(it != tables.end() && "must be visited");
+                fieldDef = it->second.lookup(field.getAttr());
+
+                if (!fieldDef) {
+                  auto error =
+                      objectField.emitOpError("referenced non-existent field ")
+                      << field;
+                  error.attachNote(classDef.getLoc()) << "class defined here";
+                  return WalkResult::interrupt();
+                }
+
+                // If there are more fields, verify the current field is of
+                // ClassType, and look up the ClassOp for that field.
+                if (i < e - 1) {
+                  auto classType = dyn_cast<ClassType>(fieldDef.getType());
+                  if (!classType) {
+                    objectField.emitOpError("nested field access into ")
+                        << field << " requires a ClassType, but found "
+                        << fieldDef.getType();
+                    return WalkResult::interrupt();
+                  }
+
+                  // Check if the nested ClassOp exists. ObjectInstOp verifier
+                  // already checked the class exits but it's not verified yet
+                  // if the object is an input argument.
+                  classDef = symbolTable.lookupNearestSymbolFrom<ClassLike>(
+                      objectField, classType.getClassName());
+
+                  if (!classDef) {
+                    objectField.emitError()
+                        << "class " << classType.getClassName()
+                        << " was not found";
+                    return WalkResult::interrupt();
+                  }
+
+                  // Proceed to the next field in the path.
+                  continue;
+                }
+
+                // On the last iteration down the path, save the final field
+                // being accessed.
+                finalField = fieldDef;
+              }
+
+              // Verify the accessed field type matches the result type.
+              if (finalField.getType() != objectField.getResult().getType()) {
+                objectField.emitOpError("expected type ")
+                    << objectField.getResult().getType()
+                    << ", but accessed field has type " << finalField.getType();
+
+                return WalkResult::interrupt();
+              }
+              return WalkResult::advance();
+            });
+        return LogicalResult::failure(result.wasInterrupted());
+      });
+  if (failed(result))
+    return signalPassFailure();
+  return markAllAnalysesPreserved();
+}
+
+std::unique_ptr<mlir::Pass> circt::om::createVerifyObjectFieldsPass() {
+  return std::make_unique<VerifyObjectFieldsPass>();
+}

lib/Firtool/Firtool.cpp

@@ -244,6 +244,7 @@ LogicalResult firtool::populateLowFIRRTLToHW(mlir::PassManager &pm,
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createLowerXMRPass());
 
   pm.nest<firrtl::CircuitOp>().addPass(firrtl::createLowerClassesPass());
+  pm.nest<firrtl::CircuitOp>().addPass(om::createVerifyObjectFieldsPass());
 
   // Check for static asserts.
   pm.nest<firrtl::CircuitOp>().nest<firrtl::FModuleOp>().addPass(
@@ -261,6 +262,9 @@ LogicalResult firtool::populateLowFIRRTLToHW(mlir::PassManager &pm,
   // Check inner symbols and inner refs.
   pm.addPass(hw::createVerifyInnerRefNamespacePass());
 
+  // Check OM object fields.
+  pm.addPass(om::createVerifyObjectFieldsPass());
+
   return success();
 }
 
@@ -308,6 +312,9 @@ LogicalResult firtool::populateHWToSV(mlir::PassManager &pm,
   // Check inner symbols and inner refs.
   pm.addPass(hw::createVerifyInnerRefNamespacePass());
 
+  // Check OM object fields.
+  pm.addPass(om::createVerifyObjectFieldsPass());
+
   return success();
 }
 
@@ -341,6 +348,9 @@ populatePrepareForExportVerilog(mlir::PassManager &pm,
   // Check inner symbols and inner refs.
   pm.addPass(hw::createVerifyInnerRefNamespacePass());
 
+  // Check OM object fields.
+  pm.addPass(om::createVerifyObjectFieldsPass());
+
   return success();
 }
 } // namespace detail

test/Dialect/OM/errors.mlir

@@ -1,4 +1,4 @@
-// RUN: circt-opt %s -verify-diagnostics -split-input-file
+// RUN: circt-opt -om-verify-object-fields %s -verify-diagnostics -split-input-file
 
 om.class @Class() {
   // expected-error @+1 {{'om.object' op result type ("Bar") does not match referred to class ("Foo")}}
@@ -39,7 +39,7 @@ om.class @Class1() {}
 
 om.class @Class2() {
   %0 = om.object @Class1() : () -> !om.class.type<@Class1>
-  // expected-error @+1 {{'om.object.field' op referenced non-existant field @foo}}
+  // expected-error @+1 {{'om.object.field' op referenced non-existent field @foo}}
   om.object.field %0, [@foo] : (!om.class.type<@Class1>) -> i1
 }
 
@@ -124,3 +124,20 @@ om.class @BadPath(%basepath: !om.basepath) {
   // expected-error @below {{invalid symbol reference}}
   %0 = om.path_create reference %basepath @Thing
 }
+
+
+// -----
+
+om.class @DupField(%0: i1) {
+  // expected-note @+1 {{previous definition is here}}
+  om.class.field @foo, %0 : i1
+  // expected-error @+1 {{'om.class.field' op field "foo" is defined twice}}
+  om.class.field @foo, %0 : i1
+}
+
+// -----
+
+om.class @UnknownClass(%arg: !om.class.type<@Unknwon>) {
+  // expected-error @+1 {{class @Unknwon was not found}}
+  om.object.field %arg, [@unknown]: (!om.class.type<@Unknwon>) -> i1
+}