[FIRRTL] Dedup: properly merge inner symbols

When we deduplicate modules, we allow operations with different inner
symbol definitions to deduplicate with each other. Unfortunately the
operation merging procedure could only handle inner symbols with field
id 0, i.e. inner symbols which targeted the entire operation. If an
operation with a field-specific inner symbol was merged into another
operation, we would end up dropping the inner symbol. This change
properly handles inner symbols with many symbol definitions when merging
operations by properly merging the inner symbol property list.

Author: youngar

lib/Dialect/FIRRTL/Transforms/Dedup.cpp

@@ -1316,6 +1316,53 @@ struct Deduper {
     }
   }
 
+  hw::InnerSymAttr mergeInnerSymbols(RenameMap &renameMap, FModuleLike toModule,
+                                     hw::InnerSymAttr toSym,
+                                     hw::InnerSymAttr fromSym) {
+    if (fromSym && !fromSym.getProps().empty()) {
+      auto &isn = getNamespace(toModule);
+      // The properties for the new inner symbol..
+      SmallVector<hw::InnerSymPropertiesAttr> newProps;
+      // If the "to" op already has an inner symbol, copy all its properties.
+      if (toSym)
+        llvm::append_range(newProps, toSym);
+      // Add each property from the fromSym to the toSym.
+      for (auto fromProp : fromSym) {
+        hw::InnerSymPropertiesAttr newProp;
+        auto *it = llvm::find_if(newProps, [&](auto p) {
+          return p.getFieldID() == fromProp.getFieldID();
+        });
+        if (it != newProps.end()) {
+          // If we already have an inner sym with the same field id, use
+          // that.
+          newProp = *it;
+          // If the old symbol is public, we need to make the new one public.
+          if (fromProp.getSymVisibility().getValue() == "public" &&
+              newProp.getSymVisibility().getValue() != "public") {
+            *it = hw::InnerSymPropertiesAttr::get(context, newProp.getName(),
+                                                  newProp.getFieldID(),
+                                                  fromProp.getSymVisibility());
+          }
+        } else {
+          // We need to add a new property to the inner symbol for this field.
+          auto newName = isn.newName(fromProp.getName().getValue());
+          newProp = hw::InnerSymPropertiesAttr::get(
+              context, StringAttr::get(context, newName), fromProp.getFieldID(),
+              fromProp.getSymVisibility());
+          newProps.push_back(newProp);
+        }
+        renameMap[fromProp.getName()] = newProp.getName();
+      }
+      // Sort the fields by field id.
+      llvm::sort(newProps, [](auto &p, auto &q) {
+        return p.getFieldID() < q.getFieldID();
+      });
+      // Return the merged inner symbol.
+      return hw::InnerSymAttr::get(context, newProps);
+    }
+    return hw::InnerSymAttr();
+  }
+
   // Record the symbol name change of the operation or any of its ports when
   // merging two operations.  The renamed symbols are used to update the
   // target of any NLAs.  This will add symbols to the "to" operation if needed.
@@ -1324,22 +1371,20 @@ struct Deduper {
                         Operation *from) {
     // If the "from" operation has an inner_sym, we need to make sure the
     // "to" operation also has an `inner_sym` and then record the renaming.
-    if (auto fromSym = getInnerSymName(from)) {
-      auto toSym =
-          getOrAddInnerSym(to, [&](auto _) -> hw::InnerSymbolNamespace & {
-            return getNamespace(toModule);
-          });
-      renameMap[fromSym] = toSym;
+    if (auto fromInnerSym = dyn_cast<hw::InnerSymbolOpInterface>(from)) {
+      auto toInnerSym = cast<hw::InnerSymbolOpInterface>(to);
+      if (auto newSymAttr = mergeInnerSymbols(renameMap, toModule,
+                                              toInnerSym.getInnerSymAttr(),
+                                              fromInnerSym.getInnerSymAttr()))
+        toInnerSym.setInnerSymbolAttr(newSymAttr);
     }
 
     // If there are no port symbols on the "from" operation, we are done here.
     auto fromPortSyms = from->getAttrOfType<ArrayAttr>("portSymbols");
     if (!fromPortSyms || fromPortSyms.empty())
       return;
     // We have to map each "fromPort" to each "toPort".
-    auto &moduleNamespace = getNamespace(toModule);
     auto portCount = fromPortSyms.size();
-    auto portNames = to->getAttrOfType<ArrayAttr>("portNames");
     auto toPortSyms = to->getAttrOfType<ArrayAttr>("portSymbols");
 
     // Create an array of new port symbols for the "to" operation, copy in the
@@ -1351,27 +1396,12 @@ struct Deduper {
       newPortSyms.assign(toPortSyms.begin(), toPortSyms.end());
 
     for (unsigned portNo = 0; portNo < portCount; ++portNo) {
-      // If this fromPort doesn't have a symbol, move on to the next one.
-      if (!fromPortSyms[portNo])
-        continue;
-      auto fromSym = cast<hw::InnerSymAttr>(fromPortSyms[portNo]);
-
-      // If this toPort doesn't have a symbol, assign one.
-      hw::InnerSymAttr toSym;
-      if (!newPortSyms[portNo]) {
-        // Get a reasonable base name for the port.
-        StringRef symName = "inner_sym";
-        if (portNames)
-          symName = cast<StringAttr>(portNames[portNo]).getValue();
-        // Create the symbol and store it into the array.
-        toSym = hw::InnerSymAttr::get(
-            StringAttr::get(context, moduleNamespace.newName(symName)));
-        newPortSyms[portNo] = toSym;
-      } else
-        toSym = cast<hw::InnerSymAttr>(newPortSyms[portNo]);
-
-      // Record the renaming.
-      renameMap[fromSym.getSymName()] = toSym.getSymName();
+      if (auto newPortSym = mergeInnerSymbols(
+              renameMap, toModule,
+              llvm::cast_if_present<hw::InnerSymAttr>(newPortSyms[portNo]),
+              cast<hw::InnerSymAttr>(fromPortSyms[portNo]))) {
+        newPortSyms[portNo] = newPortSym;
+      }
     }
 
     // Commit the new symbol attribute.

test/Dialect/FIRRTL/dedup.mlir

@@ -161,12 +161,12 @@ firrtl.circuit "Annotations" {
 
     // Should deduplicate wires where only one has a symbol
     // CHECK: %h = firrtl.wire sym @h : !firrtl.uint
-    // CHECK: %i = firrtl.wire sym @sym : !firrtl.uint
+    // CHECK: %i = firrtl.wire sym @p : !firrtl.uint
     %h = firrtl.wire sym @h : !firrtl.uint
     %i = firrtl.wire : !firrtl.uint
 
     // Should merge multiple different symbols.
-    // CHECK: %j = firrtl.wire sym [<@sym_0,0,public>, <@j,1,private>]
+    // CHECK: %j = firrtl.wire sym [<@q,0,private>, <@j,1,private>] : !firrtl.bundle<a: uint<1>>
     %j = firrtl.wire sym [<@j, 1, private>] : !firrtl.bundle<a: uint<1>>
   }
   // CHECK-NOT: firrtl.module private @Annotations1
@@ -243,6 +243,31 @@ firrtl.circuit "PortAnnotations" {
   }
 }
 
+// Check that we are merging inner symbols correctly when deduplicating.
+// CHECK-LABEL: firrtl.circuit "InnerSymOpTarget"
+firrtl.circuit "InnerSymOpTarget" {
+  // CHECK: hw.hierpath private @path0 [@InnerSymOpTarget::@foo0, @Foo0::@A]
+  hw.hierpath private @path0 [@InnerSymOpTarget::@foo0, @Foo0::@A]
+  // CHECK: hw.hierpath private @path1 [@InnerSymOpTarget::@foo0, @Foo0::@C]
+  hw.hierpath private @path1 [@InnerSymOpTarget::@foo0, @Foo0::@C]
+  // CHECK: hw.hierpath private @path2 [@InnerSymOpTarget::@foo1, @Foo0::@A_0]
+  hw.hierpath private @path2 [@InnerSymOpTarget::@foo1, @Foo1::@A]
+  // CHECK: hw.hierpath private @path3 [@InnerSymOpTarget::@foo1, @Foo0::@C]
+  hw.hierpath private @path3 [@InnerSymOpTarget::@foo1, @Foo1::@B]
+  firrtl.module private @Foo0() {
+    // CHECK: %w0 = firrtl.wire sym [<@A,0,private>, <@C,1,public>, <@A_0,2,private>]
+    %w0 = firrtl.wire sym [<@A, 0, private>, <@C, 1, private>]: !firrtl.vector<uint<1>, 4>
+  }
+  firrtl.module private @Foo1() {
+    %w1 = firrtl.wire sym [<@A, 2, private>, <@B, 1, public>]: !firrtl.vector<uint<1>, 4>
+  }
+  firrtl.module @InnerSymOpTarget() {
+    firrtl.instance foo0 sym @foo0 @Foo0()
+    // CHECK: firrtl.instance foo1 sym @foo1 @Foo0()
+    firrtl.instance foo1 sym @foo1 @Foo1()
+  }
+}
+
 // Non-local annotations should have their path updated and bread crumbs should
 // not be turned into non-local annotations. Note that this should not create
 // totally new NLAs for the annotations, it should just update the existing
@@ -452,15 +477,15 @@ firrtl.circuit "ExtModuleTest" {
 // https://github.com/llvm/circt/issues/2713
 // CHECK-LABEL: firrtl.circuit "Foo"
 firrtl.circuit "Foo"  {
-  // CHECK: hw.hierpath private @nla_1 [@Foo::@b, @A::@a]
+  // CHECK: hw.hierpath private @nla_1 [@Foo::@b, @A::@b_0]
   hw.hierpath private @nla_1 [@Foo::@b, @B::@b]
-  // CHECK: firrtl.extmodule private @A(out a: !firrtl.clock sym @a [{circt.nonlocal = @nla_1}])
-  firrtl.extmodule private @A(out a: !firrtl.clock) attributes {defname = "a"}
-  firrtl.extmodule private @B(out a: !firrtl.clock sym @b [{circt.nonlocal = @nla_1}]) attributes {defname = "a"}
+  // CHECK: firrtl.extmodule private @A(out a: !firrtl.clock sym @b_0 [{circt.nonlocal = @nla_1}], out b: !firrtl.clock sym @b)
+  firrtl.extmodule private @A(out a: !firrtl.clock, out b: !firrtl.clock sym @b) attributes {defname = "a"}
+  firrtl.extmodule private @B(out a: !firrtl.clock sym @b [{circt.nonlocal = @nla_1}], out b: !firrtl.clock) attributes {defname = "a"}
   firrtl.module @Foo() {
-    %b0_out = firrtl.instance a @A(out a: !firrtl.clock)
-    // CHECK: firrtl.instance b sym @b  @A(out a: !firrtl.clock)
-    %b1_out = firrtl.instance b sym @b @B(out a: !firrtl.clock)
+    %a0, %b0 = firrtl.instance a @A(out a: !firrtl.clock, out b: !firrtl.clock)
+    // CHECK: firrtl.instance b sym @b @A(out a: !firrtl.clock, out b: !firrtl.clock)
+    %a1, %b1 = firrtl.instance b sym @b @B(out a: !firrtl.clock, out b: !firrtl.clock)
   }
 }