Address comments

llvm · May 13, 2024 · 545e536 · 545e536
1 parent 37f655b
commit 545e536
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 11 deletions.
diff --git a/lib/Dialect/OM/Transforms/VerifyObjectFields.cpp b/lib/Dialect/OM/Transforms/VerifyObjectFields.cpp
@@ -28,6 +28,10 @@ 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
 
@@ -44,13 +48,25 @@ void VerifyObjectFieldsPass::runOnOperation() {
     tables.insert({op, llvm::DenseMap<StringAttr, ClassFieldLike>()});
 
   // Peel tables parallelly.
-  mlir::parallelForEach(&getContext(), tables, [](auto &entry) {
-    ClassLike classLike = entry.first;
-    auto &table = entry.second;
-    classLike.walk([&](ClassFieldLike fieldLike) {
-      table.insert({fieldLike.getNameAttr(), fieldLike});
-    });
-  });
+  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(
@@ -72,13 +88,13 @@ void VerifyObjectFieldsPass::runOnOperation() {
                 // Verify the field exists on the ClassOp.
                 auto field = fields[i];
                 ClassFieldLike fieldDef;
-                auto it = tables.find(classDef);
-                assert(it != tables.end() && "must be vistied");
+                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-existant field ")
+                      objectField.emitOpError("referenced non-existent field ")
                       << field;
                   error.attachNote(classDef.getLoc()) << "class defined here";
                   return WalkResult::interrupt();

diff --git a/test/Dialect/OM/errors.mlir b/test/Dialect/OM/errors.mlir
@@ -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,13 @@ 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 @below {{'om.class.field' op field "foo" is defined twice}}
+  om.class.field @foo, %0 : i1
+}