[HW][SV] Change referencing for names inside modules (llvm#2032)

Since wires, etc don't really behave like symbols in mlir, don't claim they are symbols. Use a different attribute to name things within a module (effectively a nested symbol table), without breaking module resolution by actually using a nested symbol table.
Add a attribute to refer to names inside modules: #hw.innerNameRef.
Update bind and verbatim to take moduese or innerNameRefs, and add associated output code.
Add an argument attribute to tag a port as having a visible name and update the resolution code in ExportVerilog to be able to resolve innNameRefs to ports. This gives a unified way to name verilog-namable entities.

include/circt/Dialect/HW/HW.td

@@ -22,6 +22,7 @@ include "mlir/IR/RegionKindInterface.td"
 
 include "circt/Dialect/HW/HWDialect.td"
 include "circt/Dialect/HW/HWAttributes.td"
+include "circt/Dialect/HW/HWAttributesNaming.td"
 
 include "circt/Dialect/HW/HWTypesImpl.td"
 include "circt/Dialect/HW/HWTypes.td"

include/circt/Dialect/HW/HWAttributes.h

@@ -16,6 +16,12 @@ namespace circt {
 namespace hw {
 class PEOAttr;
 enum class PEO : uint32_t;
+
+// Eventually move this to an op trait
+struct InnerName {
+  static llvm::StringRef getInnerNameAttrName() { return "inner_sym"; }
+};
+
 } // namespace hw
 } // namespace circt
 

include/circt/Dialect/HW/HWAttributesNaming.td

@@ -0,0 +1,25 @@
+//===- HWAttributesNaming.td - Attributes for HW dialect ---*- tablegen -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines HW dialect attributes used in other dialects.
+//
+//===----------------------------------------------------------------------===//
+
+def InnerRefAttr : AttrDef<HWDialect, "InnerRef"> {
+  let summary = "Refer to a name inside a module";
+  let description = [{
+    This works like a symbol reference, but to a name inside a module.
+  }];
+  let mnemonic = "innerNameRef";
+  let parameters = (ins "::mlir::StringAttr":$module, "::mlir::StringAttr":$name);
+  let builders = [
+    AttrBuilderWithInferredContext<(ins "::mlir::StringAttr":$module, "::mlir::StringAttr":$name),[{
+      return get(module.getContext(), module, name);
+    }]>,
+  ];
+}

include/circt/Dialect/HW/HWOps.h

@@ -22,6 +22,7 @@
 #include "mlir/IR/SymbolTable.h"
 #include "mlir/Interfaces/ControlFlowInterfaces.h"
 #include "mlir/Interfaces/SideEffectInterfaces.h"
+#include "llvm/ADT/StringExtras.h"
 
 namespace circt {
 namespace hw {
@@ -148,23 +149,62 @@ LogicalResult checkParameterInContext(Attribute value, Operation *module,
 /// "freeze" method transitions between the two states.
 class SymbolCache {
 public:
+  class Item {
+  public:
+    Item(Operation *op) : op(op), port(~0ULL) {}
+    Item(Operation *op, size_t port) : op(op), port(port) {}
+    bool hasPort() const { return port != ~0ULL; }
+    Operation *getOp() const { return op; }
+    size_t getPort() const { return port; }
+
+  private:
+    Operation *op;
+    size_t port;
+  };
+
   /// In the building phase, add symbols.
   void addDefinition(StringAttr symbol, Operation *op) {
     assert(!isFrozen && "cannot mutate a frozen cache");
-    symbolCache[symbol.getValue()] = op;
+    symbolCache.try_emplace(symbol.getValue(), op, ~0UL);
+  }
+
+  // Add inner names, which might be ports
+  void addDefinition(StringAttr symbol, StringRef name, Operation *op,
+                     size_t port = ~0UL) {
+    assert(!isFrozen && "cannot mutate a frozen cache");
+    auto key = mkInnerKey(symbol.getValue(), name);
+    symbolCache.try_emplace(StringRef(key.data(), key.size()), op, port);
   }
 
   /// Mark the cache as frozen, which allows it to be shared across threads.
   void freeze() { isFrozen = true; }
 
-  Operation *getDefinition(StringAttr symbolName) const {
+  Operation *getDefinition(StringRef symbol) const {
     assert(isFrozen && "cannot read from this cache until it is frozen");
-    auto it = symbolCache.find(symbolName.getValue());
-    return it != symbolCache.end() ? it->second : nullptr;
+    auto it = symbolCache.find(symbol);
+    if (it == symbolCache.end())
+      return nullptr;
+    assert(!it->second.hasPort() && "Module names should never be ports");
+    return it->second.getOp();
+  }
+
+  Operation *getDefinition(StringAttr symbol) const {
+    return getDefinition(symbol.getValue());
   }
 
   Operation *getDefinition(FlatSymbolRefAttr symbol) const {
-    return getDefinition(symbol.getAttr());
+    return getDefinition(symbol.getValue());
+  }
+
+  Item getDefinition(StringRef symbol, StringRef name) const {
+    assert(isFrozen && "cannot read from this cache until it is frozen");
+    auto key = mkInnerKey(symbol, name);
+    auto it = symbolCache.find(StringRef(key.data(), key.size()));
+    return it == symbolCache.end() ? Item{nullptr, ~0UL} : it->second;
+  }
+
+  Item getDefinition(StringAttr symbol, StringAttr name) const {
+    return getDefinition(symbol.getValue(), name.getValue());
   }
 
 private:
@@ -173,9 +213,22 @@ class SymbolCache {
   /// This stores a lookup table from symbol attribute to the operation
   /// (hw.module, hw.instance, etc) that defines it.
   /// TODO: It is super annoying that symbols are *defined* as StringAttr, but
-  /// are then referenced as FlatSymbolRefAttr.  Why can't we have nice pointer
-  /// uniqued things?? :-(
-  llvm::StringMap<Operation *> symbolCache;
+  /// are then referenced as FlatSymbolRefAttr.  Why can't we have nice
+  /// pointer uniqued things?? :-(
+  llvm::StringMap<Item> symbolCache;
+
+  // Construct a string key with embedded null.  StringMapImpl::FindKey uses
+  // explicit lengths and stores keylength, rather than relying on null
+  // characters.
+  SmallVector<char> mkInnerKey(StringRef mod, StringRef name) const {
+    assert(!mod.contains(0) && !name.contains(0) &&
+           "Null character in identifier");
+    SmallVector<char> key;
+    key.append(mod.begin(), mod.end());
+    key.push_back(0);
+    key.append(name.begin(), name.end());
+    return key;
+  }
 };
 
 } // namespace hw

include/circt/Dialect/HW/HWStructure.td

@@ -325,7 +325,7 @@ def HWModuleGeneratedOp : HWOp<"module.generated",
   let verifier = "return ::verify$cppClass(*this);";
 }
 
-def InstanceOp : HWOp<"instance", [Symbol,
+def InstanceOp : HWOp<"instance", [
         DeclareOpInterfaceMethods<SymbolUserOpInterface>,
         DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>]> {
   let summary = "Create an instance of a module";
@@ -343,7 +343,7 @@ def InstanceOp : HWOp<"instance", [Symbol,
                        Variadic<AnyType>:$inputs,
                        StrArrayAttr:$argNames, StrArrayAttr:$resultNames,
                        ParamDeclArrayAttr:$parameters,
-                       OptionalAttr<SymbolNameAttr>:$sym_name);
+                       OptionalAttr<SymbolNameAttr>:$inner_sym);
   let results = (outs Variadic<AnyType>);
 
   let builders = [

include/circt/Dialect/HW/HWTypesImpl.td

@@ -174,4 +174,3 @@ def TypeAliasType : HWType<"TypeAlias"> {
     TypedeclOp getTypeDecl(const SymbolCache &cache);
   }];
 }
-

include/circt/Dialect/SV/SV.td

@@ -51,6 +51,7 @@ def NonProceduralOp : NativeOpTrait<"NonProceduralOp"> {
 }
 
 include "circt/Dialect/HW/HWTypes.td"
+include "circt/Dialect/HW/HWAttributesNaming.td"
 include "circt/Dialect/SV/SVTypes.td"
 include "circt/Dialect/SV/SVExpressions.td"
 include "circt/Dialect/SV/SVInOutOps.td"

include/circt/Dialect/SV/SVExpressions.td

@@ -29,7 +29,7 @@ def VerbatimExprOp : SVOp<"verbatim.expr", [NoSideEffect, HasCustomSSAName]> {
     }];
 
   let arguments = (ins StrAttr:$string, Variadic<AnyType>:$operands,
-                  DefaultValuedAttr<SymRefArrayAttr,"{}">:$symbols);
+                  DefaultValuedAttr<NameRefArrayAttr,"{}">:$symbols);
   let results = (outs AnySignlessIntegerOrInOutType:$result);
   let assemblyFormat = [{
     $string (`(` $operands^ `)`)?
@@ -65,7 +65,7 @@ def VerbatimExprSEOp : SVOp<"verbatim.expr.se", [HasCustomSSAName]> {
     }];
 
   let arguments = (ins StrAttr:$string, Variadic<AnyType>:$operands,
-                 DefaultValuedAttr<SymRefArrayAttr,"{}">:$symbols );
+                 DefaultValuedAttr<NameRefArrayAttr,"{}">:$symbols );
   let results = (outs AnySignlessIntegerOrInOutType:$result);
   let assemblyFormat = [{
     $string (`(` $operands^ `)`)?

include/circt/Dialect/SV/SVInOutOps.td

@@ -12,81 +12,66 @@
 //===----------------------------------------------------------------------===//
 
 // Note that net declarations like 'wire' should not appear in an always block.
-def WireOp : SVOp<"wire", [NonProceduralOp, Symbol,
+def WireOp : SVOp<"wire", [NonProceduralOp, 
           DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>]> {
   let summary = "Define a new wire";
   let description = [{
     Declare a SystemVerilog Net Declaration of 'wire' type.
      See SV Spec 6.7, pp97.
     }];
 
-  let arguments = (ins StrAttr:$name, OptionalAttr<SymbolNameAttr>:$sym_name);
+  let arguments = (ins StrAttr:$name, OptionalAttr<SymbolNameAttr>:$inner_sym);
   let results = (outs InOutType:$result);
 
   let skipDefaultBuilders = 1;
   let builders = [
     OpBuilder<(ins "::mlir::Type":$elementType,
                       CArg<"StringAttr", "StringAttr()">:$name,
-                      CArg<"StringAttr", "StringAttr()">:$sym_name)>,
+                      CArg<"StringAttr", "StringAttr()">:$inner_sym)>,
     OpBuilder<(ins "::mlir::Type":$elementType, CArg<"StringRef">:$name), [{
       return build($_builder, $_state, elementType,
                    $_builder.getStringAttr(name));
     }]>
   ];
 
-  let assemblyFormat = [{ (`sym` $sym_name^)? custom<ImplicitSSAName>(attr-dict)
+  let assemblyFormat = [{ (`sym` $inner_sym^)? custom<ImplicitSSAName>(attr-dict)
                           `:` type($result) }];
   let hasCanonicalizeMethod = true;
 
   let extraClassDeclaration = [{
     Type getElementType() {
       return result().getType().cast<InOutType>().getElementType();
     }
-
-    //===------------------------------------------------------------------===//
-    // SymbolOpInterface Methods
-    //===------------------------------------------------------------------===//
-
-    /// An WireOp may optionally define a symbol.
-    bool isOptionalSymbol() { return true; }
-
   }];
   let verifier = [{ return ::verifyWireOp(*this); }];
 }
 
-def RegOp : SVOp<"reg", [Symbol,
+def RegOp : SVOp<"reg", [
           DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>]> {
   let summary = "Define a new `reg` in SystemVerilog";
    let description = [{
      Declare a SystemVerilog Variable Declaration of 'reg' type.
      See SV Spec 6.8, pp100.
    }];
-  let arguments = (ins StrAttr:$name, OptionalAttr<SymbolNameAttr>:$sym_name);
+  let arguments = (ins StrAttr:$name, OptionalAttr<SymbolNameAttr>:$inner_sym);
   let results = (outs InOutType:$result);
 
   let skipDefaultBuilders = 1;
   let builders = [
     OpBuilder<(ins "::mlir::Type":$elementType,
                     CArg<"StringAttr", "StringAttr()">:$name,
-                    CArg<"StringAttr", "StringAttr()">:$sym_name)>
+                    CArg<"StringAttr", "StringAttr()">:$inner_sym)>
   ];
 
   // We handle the name in a custom way, so we use a customer parser/printer.
-  let assemblyFormat = [{ (`sym` $sym_name^)? custom<ImplicitSSAName>(attr-dict)
+  let assemblyFormat = [{ (`sym` $inner_sym^)? custom<ImplicitSSAName>(attr-dict)
                           `:` type($result) }];
   let hasCanonicalizeMethod = true;
 
   let extraClassDeclaration = [{
     Type getElementType() {
       return result().getType().cast<InOutType>().getElementType();
     }
-
-    //===------------------------------------------------------------------===//
-    // SymbolOpInterface Methods
-    //===------------------------------------------------------------------===//
-
-    /// An RegOp may optionally define a symbol.
-    bool isOptionalSymbol() { return true; }
   }];
 }
 

include/circt/Dialect/SV/SVOps.h

@@ -23,6 +23,7 @@ namespace circt {
 namespace hw {
 class InstanceOp;
 class SymbolCache;
+class InnerRefAttr;
 } // namespace hw
 
 namespace sv {

include/circt/Dialect/SV/SVStatements.td

@@ -440,7 +440,7 @@ def VerbatimOp : SVOp<"verbatim"> {
     sv.verbatim allows operand substitutions with {{0}} syntax.
   }];
   let arguments = (ins StrAttr:$string, Variadic<AnyType>:$operands,
-                  DefaultValuedAttr<SymRefArrayAttr,"{}">:$symbols);
+                  DefaultValuedAttr<NameRefArrayAttr,"{}">:$symbols);
 
   let assemblyFormat =
     "$string (`(` $operands^ `)` `:` type($operands))? attr-dict";
@@ -473,12 +473,14 @@ def BindOp : SVOp<"bind", []> {
   }];
 
   let arguments =
-   (ins FlatSymbolRefAttr:$boundInstance, FlatSymbolRefAttr:$instanceModule);
+   (ins InnerRefAttr:$instance);
   let results = (outs);
   let verifier = "return ::verify$cppClass(*this);";
 
-  let assemblyFormat = "$boundInstance `in` $instanceModule attr-dict";
-
+  let assemblyFormat = "$instance attr-dict";
+  let builders = [
+    OpBuilder<(ins "StringAttr":$mod, "StringAttr":$name)>
+  ];
   let extraClassDeclaration = [{
     /// Lookup the instance for the bind.  This returns null on invalid IR.
     hw::InstanceOp getReferencedInstance(const hw::SymbolCache *cache = nullptr);

include/circt/Dialect/SV/SVTypes.td

@@ -44,7 +44,7 @@ class InOutElementConstraint<string value, string inoutValue>
                    inoutValue, value, "sv::getInOutElementType($_self)">;
 
 // This type is an array of FlatSymbolRefAttr.
-//TODO: This should be moved to mlir, it does not depend on HW dialect.
+// TODO: This should be moved to mlir, it does not depend on HW dialect.
 def SymRefArrayAttr: ArrayAttrBase<
     And<[
       // Guarantee this is an ArrayAttr first
@@ -57,6 +57,19 @@ def SymRefArrayAttr: ArrayAttrBase<
   let constBuilderCall = "$_builder.getArrayAttr($0)";
 }
 
+// Like a SymRefArrayAttr, but can also refer to names inside modules.
+def NameRefArrayAttr: ArrayAttrBase<
+    And<[
+      // Guarantee this is an ArrayAttr first
+      CPred<"$_self.isa<::mlir::ArrayAttr>()">,
+      // Guarantee all elements are symbols or name refs
+      CPred<"::llvm::all_of($_self.cast<::mlir::ArrayAttr>(), "
+            "[&](::mlir::Attribute attr) { return attr.isa<"
+            "::mlir::FlatSymbolRefAttr, ::hw::InnerRefAttr>();})">]>,
+    ""> {
+  let constBuilderCall = "$_builder.getArrayAttr($0)";
+}
+
 def StringArrayAttr: ArrayAttrBase<
     And<[
       // Guarantee this is an ArrayAttr first
@@ -67,4 +80,5 @@ def StringArrayAttr: ArrayAttrBase<
             "::mlir::StringAttr>();})">]>,
     ""> {
   let constBuilderCall = "$_builder.getArrayAttr($0)";
-}
+}
+