[FIRRTL][Dedup] Improve diagnostic for failing to dedup due to public.

lib/Dialect/FIRRTL/Transforms/Dedup.cpp

@@ -46,6 +46,20 @@ using namespace circt;
 using namespace firrtl;
 using hw::InnerRefAttr;
 
+//===----------------------------------------------------------------------===//
+// Utility function for classifying a Symbol's dedup-ability.
+//===----------------------------------------------------------------------===//
+
+static bool checkVisibility(mlir::SymbolOpInterface symbol) {
+  // If module has symbol (name) that must be preserved even if unused,
+  // skip it. All symbol uses must be supported, which is not true if
+  // non-private.
+  // Special handling for class-like's and if symbol reports cannot be
+  // discarded.
+  return symbol.isPrivate() &&
+         (symbol.canDiscardOnUseEmpty() || isa<ClassLike>(*symbol));
+}
+
 //===----------------------------------------------------------------------===//
 // Hashing
 //===----------------------------------------------------------------------===//
@@ -788,6 +802,20 @@ struct Equivalence {
       diag.attachNote(b->getLoc()) << "module marked NoDedup";
       return;
     }
+    auto aSymbol = cast<mlir::SymbolOpInterface>(a);
+    auto bSymbol = cast<mlir::SymbolOpInterface>(b);
+    if (!checkVisibility(aSymbol)) {
+      diag.attachNote(a->getLoc())
+          << "module is "
+          << (aSymbol.isPrivate() ? "private but not discardable" : "public");
+      return;
+    }
+    if (!checkVisibility(bSymbol)) {
+      diag.attachNote(b->getLoc())
+          << "module is "
+          << (bSymbol.isPrivate() ? "private but not discardable" : "public");
+      return;
+    }
     auto aGroup =
         dyn_cast_or_null<StringAttr>(a->getDiscardableAttr(dedupGroupAttrName));
     auto bGroup = dyn_cast_or_null<StringAttr>(
@@ -1674,13 +1702,8 @@ class DedupPass : public circt::firrtl::impl::DedupBase<DedupPass> {
               ext && !ext.getDefname().has_value())
             return success();
 
-          // If module has symbol (name) that must be preserved even if unused,
-          // skip it. All symbol uses must be supported, which is not true if
-          // non-private.
-          if (!module.isPrivate() ||
-              (!module.canDiscardOnUseEmpty() && !isa<ClassLike>(*module))) {
+          if (!checkVisibility(module))
             return success();
-          }
 
           StructuralHasher hasher(hasherConstants);
           // Calculate the hash of the module and referred module names.

test/Dialect/FIRRTL/dedup-errors.mlir

@@ -630,3 +630,56 @@ firrtl.circuit "MustDedup" attributes {annotations = [{
     firrtl.instance test1 @Test1(in in : !firrtl.vector<uint<1>, 2>)
   }
 }
+
+// -----
+
+// Modules instantiating equivalent public modules do not dedup.
+// Check diagnostic.
+// expected-error @below {{module "Test1Parent" not deduplicated with "Test0Parent"}}
+// expected-note @below {{in instance "test0" of "Test0", and instance "test1" of "Test1"}}
+firrtl.circuit "InstOfEquivPublic" attributes {annotations = [{
+      class = "firrtl.transforms.MustDeduplicateAnnotation",
+      modules = ["~InstOfEquivPublic|Test0Parent", "~InstOfEquivPublic|Test1Parent"]
+    }]} {
+  // expected-note @below {{module is public}}
+  firrtl.module public @Test0() { }
+  firrtl.module public @Test1() { }
+  firrtl.module private @Test0Parent() {
+    firrtl.instance test0 @Test0()
+  }
+  firrtl.module private @Test1Parent() {
+    firrtl.instance test1 @Test1()
+  }
+
+  firrtl.module @InstOfEquivPublic() {
+    firrtl.instance test0p @Test0Parent()
+    firrtl.instance test1p @Test1Parent()
+  }
+}
+
+// -----
+
+// Modules instantiating mixed public/private do not dedup.
+// Check diagnostic.
+// expected-error @below {{module "Test1Parent" not deduplicated with "Test0Parent"}}
+// expected-note @below {{in instance "test0" of "Test0", and instance "test1" of "Test1"}}
+firrtl.circuit "InstOfPublicPrivate" attributes {annotations = [{
+      class = "firrtl.transforms.MustDeduplicateAnnotation",
+      modules = ["~InstOfPublicPrivate|Test0Parent", "~InstOfPublicPrivate|Test1Parent"]
+    }]} {
+  firrtl.module private @Test0() { }
+  // expected-note @below {{module is public}}
+  firrtl.module public @Test1() { }
+
+  firrtl.module private @Test0Parent() {
+    firrtl.instance test0 @Test0()
+  }
+  firrtl.module private @Test1Parent() {
+    firrtl.instance test1 @Test1()
+  }
+
+  firrtl.module @InstOfPublicPrivate() {
+    firrtl.instance test0p @Test0Parent()
+    firrtl.instance test1p @Test1Parent()
+  }
+}