[ExportVerilog] Change legalization of module names to be efficient.

Instead of walking the entire design hierarchy to rename a module that
conflicts with a symbol, just add a "verilogName" attribute to it.  This
encodes a simple idea that the verilog name of a module is the contents
of the verilogName attribute or the symbol name if not present, just
like it already is for external modules.

include/circt/Dialect/HW/HWOps.h

@@ -103,6 +103,9 @@ FunctionType getModuleType(Operation *module);
 /// Returns the verilog module name attribute or symbol name of any module-like
 /// operations.
 StringAttr getVerilogModuleNameAttr(Operation *module);
+inline StringRef getVerilogModuleName(Operation *module) {
+  return getVerilogModuleNameAttr(module).getValue();
+}
 
 /// Return the port name for the specified argument or result.  These can only
 /// return a null StringAttr when the IR is invalid.

lib/Conversion/ExportVerilog/ExportVerilog.cpp

@@ -120,11 +120,8 @@ static bool isDuplicatableNullaryExpression(Operation *op) {
 static StringRef getSymOpName(Operation *symOp) {
   // Typeswitch of operation types which can define a symbol.
   return TypeSwitch<Operation *, StringRef>(symOp)
-      .Case<HWModuleOp>([&](HWModuleOp op) { return op.getName(); })
-      .Case<HWModuleExternOp>(
-          [&](HWModuleExternOp op) { return op.getVerilogModuleName(); })
-      .Case<HWGeneratorSchemaOp>(
-          [&](HWGeneratorSchemaOp op) { return op.sym_name(); })
+      .Case<HWModuleOp, HWModuleExternOp, HWModuleGeneratedOp>(
+          [](Operation *op) { return getVerilogModuleName(op); })
       .Case<InstanceOp>([&](InstanceOp op) { return op.getName().getValue(); })
       .Case<WireOp>([&](WireOp op) { return op.name(); })
       .Case<RegOp>([&](RegOp op) { return op.name(); })
@@ -2775,8 +2772,7 @@ LogicalResult StmtEmitter::visitStmt(InstanceOp op) {
   // Use the specified name or the symbol name as appropriate.
   auto *moduleOp = op.getReferencedModule(&state.symbolCache);
   assert(moduleOp && "Invalid IR");
-  auto verilogName = getVerilogModuleNameAttr(moduleOp);
-  indent() << prefix << verilogName.getValue();
+  indent() << prefix << getVerilogModuleName(moduleOp);
 
   // If this is a parameterized module, then emit the parameters.
   if (!op.parameters().empty()) {
@@ -3348,7 +3344,7 @@ void ModuleEmitter::emitHWModule(HWModuleOp module) {
   SmallPtrSet<Operation *, 8> moduleOpSet;
   moduleOpSet.insert(module);
 
-  os << "module " << module.getName();
+  os << "module " << getVerilogModuleName(module);
 
   // If we have any parameters, print them on their own line.
   if (!module.parameters().empty()) {
@@ -3743,7 +3739,7 @@ void SharedEmitterState::gatherFiles(bool separateModules) {
 
           // Emit into a separate file named after the module.
           if (attr || separateModules)
-            separateFile(mod, mod.getName() + ".sv");
+            separateFile(mod, getVerilogModuleName(mod) + ".sv");
           else
             rootFile.ops.push_back(info);
         })

lib/Conversion/ExportVerilog/LegalizeNames.cpp

@@ -50,8 +50,6 @@ public:
 
   GlobalNameTable takeGlobalNameTable() { return std::move(globalNameTable); }
 
-  bool anythingChanged = false;
-
 private:
   /// Check to see if the port names of the specified module conflict with
   /// keywords or themselves.  If so, add the replacement names to
@@ -74,11 +72,6 @@ private:
 /// Construct a GlobalNameResolver and do the initial scan to populate and
 /// unique the module/interfaces and port/parameter names.
 GlobalNameResolver::GlobalNameResolver(mlir::ModuleOp topLevel) {
-  // This symbol table is lazily constructed when global rewrites of module or
-  // interface member names are required.
-  mlir::SymbolTableCollection symbolTable;
-  Optional<mlir::SymbolUserMap> symbolUsers;
-
   // Register the names of external modules which we cannot rename. This has to
   // occur in a first pass separate from the modules and interfaces which we are
   // actually allowed to rename, in order to ensure that we don't accidentally
@@ -87,7 +80,7 @@ GlobalNameResolver::GlobalNameResolver(mlir::ModuleOp topLevel) {
     // Note that external modules *often* have name collisions, because they
     // correspond to the same verilog module with different parameters.
     if (isa<HWModuleExternOp>(op) || isa<HWModuleGeneratedOp>(op)) {
-      auto name = hw::getVerilogModuleNameAttr(&op).getValue();
+      auto name = getVerilogModuleNameAttr(&op).getValue();
       if (!sv::isNameValid(name))
         op.emitError("name \"")
             << name << "\" is not allowed in Verilog output";
@@ -95,30 +88,9 @@ GlobalNameResolver::GlobalNameResolver(mlir::ModuleOp topLevel) {
     }
   }
 
-  // If the module's symbol itself conflicts, then rename it and all uses of it.
-  auto legalizeSymbolName = [&](Operation *op,
-                                NameCollisionResolver &resolver) {
-    StringAttr oldName = SymbolTable::getSymbolName(op);
-    auto newName = resolver.getLegalName(oldName);
-    if (newName == oldName.getValue())
-      return;
-
-    // Lazily construct the symbol table if it hasn't been built yet.
-    if (!symbolUsers.hasValue())
-      symbolUsers.emplace(symbolTable, topLevel);
-
-    // TODO: This is super inefficient, we should just rename the symbol as part
-    // of the other existing walks.
-    auto newNameAttr = StringAttr::get(topLevel.getContext(), newName);
-    symbolUsers->replaceAllUsesWith(op, newNameAttr);
-    SymbolTable::setSymbolName(op, newNameAttr);
-    anythingChanged = true;
-  };
-
   // Legalize module and interface names.
   for (auto &op : *topLevel.getBody()) {
     if (auto module = dyn_cast<HWModuleOp>(op)) {
-      legalizeSymbolName(module, globalNameResolver);
       legalizeModuleNames(module);
       continue;
     }
@@ -136,6 +108,14 @@ GlobalNameResolver::GlobalNameResolver(mlir::ModuleOp topLevel) {
 /// keywords or themselves.  If so, add the replacement names to
 /// globalNameTable.
 void GlobalNameResolver::legalizeModuleNames(HWModuleOp module) {
+  // If the module's symbol itself conflicts, then set a "verilogName" attribute
+  // on the module to reflect the name we need to use.
+  StringRef oldName = module.getName();
+  auto newName = globalNameResolver.getLegalName(oldName);
+  if (newName != oldName)
+    module->setAttr("verilogName",
+                    StringAttr::get(module.getContext(), newName));
+
   NameCollisionResolver nameResolver;
 
   // Legalize the port names.

lib/Dialect/SV/Transforms/HWExportModuleHierarchy.cpp

@@ -42,17 +42,16 @@ struct HWExportModuleHierarchyPass
 /// Recursively print the module hierarchy as serialized as JSON.
 static void printHierarchy(hw::InstanceOp &inst, SymbolTable &symbolTable,
                            llvm::json::OStream &J) {
+  auto moduleOp = symbolTable.lookup(inst.moduleNameAttr().getValue());
+
   J.object([&] {
     J.attribute("instance_name", inst.instanceName());
-    J.attribute("module_name", inst.moduleName());
+    J.attribute("module_name", hw::getVerilogModuleName(moduleOp));
     J.attributeArray("instances", [&] {
-      auto moduleOp =
-          symbolTable.lookup<hw::HWModuleOp>(inst.moduleNameAttr().getValue());
-
       // Only recurse on module ops, not extern or generated ops, whose internal
       // are opaque.
-      if (moduleOp) {
-        for (auto op : moduleOp.getOps<hw::InstanceOp>()) {
+      if (auto module = dyn_cast<hw::HWModuleOp>(moduleOp)) {
+        for (auto op : module.getOps<hw::InstanceOp>()) {
           printHierarchy(op, symbolTable, J);
         }
       }
@@ -70,7 +69,7 @@ static void extractHierarchyFromTop(hw::HWModuleOp op, SymbolTable &symbolTable,
   // since the top-level module is not instantiated.
   J.object([&] {
     J.attribute("instance_name", op.getName());
-    J.attribute("module_name", op.getName());
+    J.attribute("module_name", hw::getVerilogModuleName(op));
     J.attributeArray("instances", [&] {
       for (auto op : op.getOps<hw::InstanceOp>())
         printHierarchy(op, symbolTable, J);

test/ExportVerilog/name-legalize.mlir

@@ -160,8 +160,6 @@ hw.module @verbatim_renames(%a: i1) {
   sv.verbatim "// VERB Instance : {{0}} {{1}}" {symbols = [@struct, @wire1]}
 }
 
-
-
 // CHECK-LABEL: interface output_2;
 sv.interface @output {
   // CHECK-NEXT: logic input_0;

test/ExportVerilog/sv-dialect.mlir

@@ -815,11 +815,11 @@ hw.module @verbatim_M1(%clock : i1, %cond : i1, %val : i8) {
   %c42_2 = hw.constant 42 : i8
   %xor = comb.xor %val, %c42_2 : i8
   hw.instance "aa1" sym @verbatim_b1 @verbatim_inout_2() ->()
-  // CHECK: MACRO(val + 8'h2A, val ^ 8'h2A reg=reg1, verbatim_M2, verbatim_inout_2, verbatim_schema~aa1,reg2 = reg2 )
-  sv.verbatim  "MACRO({{0}}, {{1}} reg={{2}}, {{3}}, {{4}}, {{5}}~{{6}},reg2 = {{7}} )" 
+  // CHECK: MACRO(val + 8'h2A, val ^ 8'h2A reg=reg1, verbatim_M2, verbatim_inout_2, aa1,reg2 = reg2 )
+  sv.verbatim  "MACRO({{0}}, {{1}} reg={{2}}, {{3}}, {{4}}, {{5}},reg2 = {{6}} )" 
           (%add, %xor)  : i8,i8
           {symbols = [@verbatim_reg1, @verbatim_M2, 
-          @verbatim_inout_2, @verbatim_schema, @verbatim_b1, @verbatim_reg2]}
+          @verbatim_inout_2, @verbatim_b1, @verbatim_reg2]}
   // CHECK: Wire : wire25
   sv.verbatim " Wire : {{0}}" {symbols = [@verbatim_wireSym1]}
 }