[LowerToHW] Fix symbol creation for empty names (#6282)

firrtl.instance with lowerToBind is lowered into bound instance and symbols are created for the instances. There was a bug that symbols were generated by instance names, not by an appropriate helper so there could have been name collisions. This commit fixes the issue by using inner symbol generation helper.

This also fixes symbol creation for empty port names as well.

lib/Conversion/FIRRTLToHW/LowerToHW.cpp

@@ -885,7 +885,9 @@ FIRRTLModuleLowering::lowerPorts(ArrayRef<PortInfo> firrtlPorts,
   ports.reserve(firrtlPorts.size());
   size_t numArgs = 0;
   size_t numResults = 0;
-  for (auto firrtlPort : firrtlPorts) {
+  for (auto e : llvm::enumerate(firrtlPorts)) {
+    PortInfo firrtlPort = e.value();
+    size_t portNo = e.index();
     hw::PortInfo hwPort;
     hwPort.name = firrtlPort.name;
     hwPort.type = loweringState.lowerType(firrtlPort.type, firrtlPort.loc);
@@ -907,10 +909,12 @@ FIRRTLModuleLowering::lowerPorts(ArrayRef<PortInfo> firrtlPorts,
         }
         continue;
       }
+
       hwPort.setSym(
           hw::InnerSymAttr::get(StringAttr::get(
-              moduleOp->getContext(), Twine("__") + moduleName + Twine("__") +
-                                          firrtlPort.name.strref())),
+              moduleOp->getContext(),
+              Twine("__") + moduleName + Twine("__DONTTOUCH__") +
+                  Twine(portNo) + Twine("__") + firrtlPort.name.strref())),
           moduleOp->getContext());
     }
 
@@ -3214,8 +3218,10 @@ LogicalResult FIRRTLLowering::visitDecl(InstanceOp oldInstance) {
   auto innerSym = oldInstance.getInnerSymAttr();
   if (oldInstance.getLowerToBind()) {
     if (!innerSym)
-      innerSym = hw::InnerSymAttr::get(
-          builder.getStringAttr("__" + oldInstance.getName() + "__"));
+      std::tie(innerSym, std::ignore) = getOrAddInnerSym(
+          oldInstance.getContext(), oldInstance.getInnerSymAttr(), 0,
+          [&]() -> hw::InnerSymbolNamespace & { return moduleNamespace; });
+
     auto bindOp = builder.create<sv::BindOp>(theModule.getNameAttr(),
                                              innerSym.getSymName());
     // If the lowered op already had output file information, then use that.

test/Conversion/FIRRTLToHW/lower-to-hw-module.mlir

@@ -286,11 +286,19 @@ firrtl.circuit "Simple" {
 
   // DontTouch on ports becomes symbol.
   // CHECK-LABEL: hw.module.extern private @PortDT
-  // CHECK-SAME: (in %a : i1 {hw.exportPort = #hw<innerSym@__PortDT__a>}, in %hassym : i1 {hw.exportPort = #hw<innerSym@hassym>},
-  // CHECK-SAME: out b : i2 {hw.exportPort = #hw<innerSym@__PortDT__b>})
+  // CHECK-SAME: (in %a : i1 {hw.exportPort = #hw<innerSym@__PortDT__DONTTOUCH__0__a>}, in %hassym : i1 {hw.exportPort = #hw<innerSym@hassym>},
+  // CHECK-SAME: out b : i2 {hw.exportPort = #hw<innerSym@__PortDT__DONTTOUCH__2__b>})
   firrtl.extmodule private @PortDT(
     in a: !firrtl.uint<1> [{class = "firrtl.transforms.DontTouchAnnotation"}],
     in hassym: !firrtl.uint<1> sym @hassym [{class = "firrtl.transforms.DontTouchAnnotation"}],
     out b: !firrtl.uint<2> [{class = "firrtl.transforms.DontTouchAnnotation"}]
   )
+
+  // CHECK-LABEL: @PortDT2
+  // CHECK-SAME:  {hw.exportPort = #hw<innerSym@__PortDT2__DONTTOUCH__0__>}
+  // CHECK-SAME:  {hw.exportPort = #hw<innerSym@__PortDT2__DONTTOUCH__1__>}
+  firrtl.module private @PortDT2(
+    in %0: !firrtl.uint<1> [{class = "firrtl.transforms.DontTouchAnnotation"}],
+    in %1: !firrtl.uint<1> [{class = "firrtl.transforms.DontTouchAnnotation"}]
+  ) {}
 }

test/Conversion/FIRRTLToHW/lower-to-hw.mlir

@@ -589,15 +589,19 @@ firrtl.circuit "Simple"   attributes {annotations = [{class =
     %1455 = builtin.unrealized_conversion_cast %hits_1_7 : !firrtl.uint<1> to !firrtl.uint<1>
   }
 
-  // CHECK: sv.bind <@bindTest::@[[bazSymbol:.+]]>
+  // CHECK: sv.bind <@bindTest::@[[bazSymbol:sym]]>
   // CHECK-NOT: output_file
+  // CHECK: sv.bind <@bindTest::@[[bazSymbol2:sym_0]]>
   // CHECK-NEXT: sv.bind <@bindTest::@[[quxSymbol:.+]]> {
   // CHECK-SAME: output_file = #hw.output_file<"bindings.sv", excludeFromFileList>
   // CHECK-NEXT: hw.module private @bindTest(in %dummy : i1)
   firrtl.module private @bindTest(in %dummy: !firrtl.uint<1>) {
     // CHECK: hw.instance "baz" sym @[[bazSymbol]] @bar
     %baz = firrtl.instance baz {lowerToBind} @bar(in io_cpu_flush: !firrtl.uint<1>)
     firrtl.connect %baz, %dummy : !firrtl.uint<1>, !firrtl.uint<1>
+    // CHECK: hw.instance "baz" sym @[[bazSymbol2]] @bar
+    %baz_dup = firrtl.instance baz {lowerToBind} @bar(in io_cpu_flush: !firrtl.uint<1>)
+    firrtl.connect %baz_dup, %dummy : !firrtl.uint<1>, !firrtl.uint<1>
 
     // CHECK: hw.instance "qux" sym @[[quxSymbol]] @bar
     %qux = firrtl.instance qux {lowerToBind, output_file = #hw.output_file<"bindings.sv", excludeFromFileList>} @bar(in io_cpu_flush: !firrtl.uint<1>)