[ExportVerilog] Collect names from declarations before expressions. (#3342)

A situation can occur where a named declaration has its name re-named,
in which case the hw.verilogName is used to indicate the new
name. However, it is possible for collectNames to see a certain
expression that it wants to emit into a temporary, and decides to give
it a generated name that collides with another declaration's generated
hw.verilogName. This can happen if the expression is visited before
the declaration, and results in the declaration receiving a different
unique name, which doesn't match the assigned hw.verilogName.

This isn't a huge problem for local emission, but non-local things
like binds have to be able to trust the hw.verilogName attribute
holds the final name.

The change in this patch splits collectNames to loop over the IR
twice. First collecting names from declarations that may have a
hw.verilogName, and then collecting the rest of the names, and
doing the rest of the work like measuring widths, marking values
to emit, etc.

Some implementation alternatives considered:
  * Compute some sort of data structure containing the names. We're
  moving away from global name data structures in favor of using the
  IR. Computing something locally would need a pass over the IR
  anyway, so which would trade memory for runtime.
  * Modify the hw.verilogName attr to match what ExportVerilog
  selects. We are actively trying to make ExportVerilog mutate the IR
  less, and make hw.verlogName more stable, and this would be the
  opposite.

lib/Conversion/ExportVerilog/ExportVerilog.cpp

@@ -2337,20 +2337,37 @@ void NameCollector::collectNames(Block &block) {
 
   SmallString<32> nameTmp;
 
-  // Loop over all of the results of all of the ops.  Anything that defines a
-  // value needs to be noticed.
+  // Pre-pass loop to first add any names that could be the result of re-naming.
+  // These constructs will have their names added regardless, and handling them
+  // first ensures any out of line expressions won't trample on names selected
+  // by re-naming. This could be combined into one pass through the IR that
+  // collects a worklist of exprs to re-visit instead of the double traversal.
   for (auto &op : block) {
-    // Instances have a instance name to recognize but we don't need to look
-    // at the result values and don't need to schedule them as valuesToEmit.
     if (auto instance = dyn_cast<InstanceOp>(op)) {
       names.addName(&op, getSymOpName(instance));
       continue;
     }
+
     if (auto interface = dyn_cast<InterfaceInstanceOp>(op)) {
       names.addName(interface.getResult(), getSymOpName(interface));
       continue;
     }
 
+    if (isa<WireOp, RegOp, LocalParamOp>(op)) {
+      names.addName(op.getResult(0), getSymOpName(&op));
+      continue;
+    }
+  }
+
+  // Loop over all of the results of all of the ops. Anything that defines a
+  // value needs to be noticed.
+  for (auto &op : block) {
+    // Instances have an instance name to recognize but we don't need to look
+    // at the result values and don't need to schedule them as valuesToEmit.
+    // They already had their names added in the first loop, and can be skipped.
+    if (isa<InstanceOp, InterfaceInstanceOp>(op))
+      continue;
+
     bool isExpr = isVerilogExpression(&op);
     bool isInlineExpr =
         isExpr && isExpressionEmittedInline(&op, moduleEmitter.state.options);
@@ -2396,12 +2413,6 @@ void NameCollector::collectNames(Block &block) {
       maxTypeWidth = std::max(typeString.size(), maxTypeWidth);
     }
 
-    // Notice and renamify named declarations.
-    if (isa<WireOp, RegOp, LocalParamOp>(op)) {
-      names.addName(op.getResult(0), getSymOpName(&op));
-      continue;
-    }
-
     // Notice and renamify the labels on verification statements.
     if (isa<AssertOp, AssumeOp, CoverOp, AssertConcurrentOp, AssumeConcurrentOp,
             CoverConcurrentOp>(op)) {

test/Conversion/ExportVerilog/sv-dialect.mlir

@@ -1420,6 +1420,16 @@ hw.module @Verilator3405(
   hw.output %out : i2
 }
 
+// CHECK-LABEL: module CollectNamesOrder
+hw.module @CollectNamesOrder(%in: i1) -> (out: i1) {
+  // CHECK: wire _GEN;
+  // CHECK: wire [[EXTRA_GEN:.+]] = {{.+}};
+  // CHECK: assign {{.+}} = [[EXTRA_GEN]]
+  %0 = comb.or %in, %in : i1
+  %1 = comb.or %0, %0 : i1
+  %foo = sv.wire {hw.verilogName = "_GEN" } : !hw.inout<i1>
+  hw.output %1 : i1
+}
 
 hw.module @bindInMod() {
   sv.bind #hw.innerNameRef<@remoteInstDut::@bindInst>