From d759856da75134158e8e61208e8900481f2a5711 Mon Sep 17 00:00:00 2001
From: Mike Urbach <mikeurbach@gmail.com>
Date: Tue, 14 Jun 2022 13:18:22 -0600
Subject: [PATCH] [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.
---
 .../ExportVerilog/ExportVerilog.cpp           | 31 +++++++++++++------
 test/Conversion/ExportVerilog/sv-dialect.mlir | 10 ++++++
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/lib/Conversion/ExportVerilog/ExportVerilog.cpp b/lib/Conversion/ExportVerilog/ExportVerilog.cpp
index ea3552f545..a2f864a7d4 100644
--- a/lib/Conversion/ExportVerilog/ExportVerilog.cpp
+++ b/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)) {
diff --git a/test/Conversion/ExportVerilog/sv-dialect.mlir b/test/Conversion/ExportVerilog/sv-dialect.mlir
index 04eb711804..c6e9769996 100644
--- a/test/Conversion/ExportVerilog/sv-dialect.mlir
+++ b/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>