[HW] round trip ModuleType non-ssa values (#6287)

Treat blockargs like other values and let the asm name hint interface compute names for them. If the port name is a legal ssa name, it will be used, otherwise the legalization will compute a new name based on the port name. This has stable round-tripping after the first round, which is consistent with other ops which provide ssa name hints.

lib/Dialect/HW/HWOps.cpp

@@ -104,14 +104,13 @@ static void getAsmBlockArgumentNamesImpl(mlir::Region &region,
   if (region.empty())
     return;
   // Assign port names to the bbargs.
-  //  auto *module = region.getParentOp();
   auto module = cast<HWModuleOp>(region.getParentOp());
 
   auto *block = &region.front();
   for (size_t i = 0, e = block->getNumArguments(); i != e; ++i) {
     auto name = module.getInputName(i);
-    if (!name.empty())
-      setNameFn(block->getArgument(i), name);
+    // Let mlir deterministically convert names to valid identifiers
+    setNameFn(block->getArgument(i), name);
   }
 }
 

lib/Dialect/HW/ModuleImplementation.cpp

@@ -242,15 +242,20 @@ ParseResult module_like_impl::parseModuleFunctionSignature(
 ////////////////////////////////////////////////////////////////////////////////
 
 /// Parse an optional keyword or string and set instance into 'result'.`
+/// Returns failure on a parse issue, but not on not finding the string. 'found'
+/// indicates whether the optional value exists.
 ParseResult parseOptionalKeywordOrOptionalString(OpAsmParser &p,
-                                                 std::string *result) {
+                                                 std::string &result,
+                                                 bool &found) {
   StringRef keyword;
   if (succeeded(p.parseOptionalKeyword(&keyword))) {
-    *result = keyword.str();
+    result = keyword.str();
+    found = true;
     return success();
   }
 
-  (void)p.parseOptionalString(result);
+  if (succeeded(p.parseOptionalString(&result)))
+    found = true;
   return success();
 }
 
@@ -277,9 +282,18 @@ static ParseResult parseInputPort(OpAsmParser &parser,
   NamedAttrList attrs;
 
   // Parse the result name.
-  if (parseOptionalKeywordOrOptionalString(parser, &result.rawName))
+  bool found = false;
+  if (parseOptionalKeywordOrOptionalString(parser, result.rawName, found))
     return failure();
 
+  // If there is only a ssa name, use it as the port name.  The ssa name is
+  // always required, but if there is the optional arbitrary name, it is used as
+  // the port name and the ssa name is just used for parsing the module.
+  if (!found)
+    result.rawName =
+        parsing_util::getNameFromSSA(parser.getContext(), result.ssaName.name)
+            .str();
+
   if (parser.parseColonType(result.type) ||
       parser.parseOptionalAttrDict(attrs) ||
       parser.parseOptionalLocationSpecifier(result.sourceLoc))
@@ -353,10 +367,8 @@ ParseResult module_like_impl::parseModuleSignature(
   // Process the ssa args for the information we're looking for.
   SmallVector<ModulePort> ports;
   for (auto &arg : args) {
-    std::string name = arg.rawName;
-    if (arg.direction != ModulePort::Output)
-      name = parsing_util::getNameFromSSA(context, arg.ssaName.name).str();
-    ports.push_back({StringAttr::get(context, name), arg.type, arg.direction});
+    ports.push_back(
+        {StringAttr::get(context, arg.rawName), arg.type, arg.direction});
     // rewrite type AFTER constructing ports.  This will be used in block args.
     if (arg.direction == ModulePort::InOut)
       arg.type = InOutType::get(arg.type);

test/Dialect/Comb/canonicalization.mlir

@@ -266,7 +266,7 @@ hw.module @orConcatsWithMux(in %bit: i1, in %cond: i1, out o: i6) {
 hw.module @extractNested(in %0: i5, out o1 : i1) {
 // Multiple layers of nested extract is a weak evidence that the cannonicalization
 // operates recursively.
-// CHECK-NEXT: %0 = comb.extract %arg0 from 4 : (i5) -> i1
+// CHECK-NEXT: %1 = comb.extract %0 from 4 : (i5) -> i1
   %1 = comb.extract %0 from 1 : (i5) -> i4
   %2 = comb.extract %1 from 2 : (i4) -> i2
   %3 = comb.extract %2 from 1 : (i2) -> i1

test/Dialect/DC/roundtrip.mlir

@@ -1,16 +1,16 @@
 // RUN: circt-opt %s | circt-opt | FileCheck %s
 
-// CHECK:       hw.module @foo(in %arg0 : !dc.token, in %arg1 : !dc.value<i1>, in %arg2 : i32) {
-// CHECK-NEXT:    %0 = dc.buffer[2] %arg0 : !dc.token
-// CHECK-NEXT:    %1 = dc.buffer[2] %arg1 [1, 2] : !dc.value<i1>
-// CHECK-NEXT:    %2:2 = dc.fork [2] %arg0 
-// CHECK-NEXT:    %3 = dc.pack %arg0, %arg2 : i32
-// CHECK-NEXT:    %4 = dc.merge %0, %arg0
-// CHECK-NEXT:    %token, %output = dc.unpack %arg1 : !dc.value<i1>
-// CHECK-NEXT:    %5 = dc.to_esi %arg0 : !dc.token
-// CHECK-NEXT:    %6 = dc.to_esi %arg1 : !dc.value<i1>
-// CHECK-NEXT:    %7 = dc.from_esi %5 : <i0>
-// CHECK-NEXT:    %8 = dc.from_esi %6 : <i1>
+// CHECK:       hw.module @foo(in %0 "" : !dc.token, in %1 "" : !dc.value<i1>, in %2 "" : i32) {
+// CHECK-NEXT:    %3 = dc.buffer[2] %0 : !dc.token
+// CHECK-NEXT:    %4 = dc.buffer[2] %1 [1, 2] : !dc.value<i1>
+// CHECK-NEXT:    %5:2 = dc.fork [2] %0 
+// CHECK-NEXT:    %6 = dc.pack %0, %2 : i32
+// CHECK-NEXT:    %7 = dc.merge %3, %0
+// CHECK-NEXT:    %token, %output = dc.unpack %1 : !dc.value<i1>
+// CHECK-NEXT:    %8 = dc.to_esi %0 : !dc.token
+// CHECK-NEXT:    %9 = dc.to_esi %1 : !dc.value<i1>
+// CHECK-NEXT:    %10 = dc.from_esi %8 : <i0>
+// CHECK-NEXT:    %11 = dc.from_esi %9 : <i1>
 // CHECK-NEXT:    hw.output
 // CHECK-NEXT:  }
 

test/Dialect/HW/basic.mlir

@@ -192,3 +192,13 @@ hw.module @fileListTest(in %arg1: i32) attributes {output_filelist = #hw.output_
 // CHECK-SAME: attributes {comment = "hello world"}
 hw.module @commentModule() attributes {comment = "hello world"} {}
 
+ // CHECK-LABEL: hw.module @Foo(in %_d2A " d*" : i1, in %0 "" : i1, out "" : i1, out "1" : i1) {
+hw.module @Foo(in %0 " d*" : i1, in %1 "" : i1, out "" : i1, out "1" : i1) {
+  hw.output %0 , %1: i1, i1
+ }
+ // CHECK-LABEL: hw.module @Bar(in %foo : i1, out "" : i1, out "" : i1) {
+hw.module @Bar(in %foo : i1, out "" : i1, out "" : i1) {
+   // CHECK-NEXT:  hw.instance "foo" @Foo(" d*": %foo: i1, "": %foo: i1) -> ("": i1, "1": i1)
+  %0, %1 = hw.instance "foo" @Foo(" d*": %foo: i1, "": %foo : i1)  -> ("" : i1, "1" : i1)
+  hw.output %0, %1 : i1, i1
+}

test/Dialect/SV/hw-extract-test-code.mlir

@@ -567,7 +567,7 @@ module {
   // CHECK-SAME: (in %clock : !seq.clock, in %port_1 : i1, in %port_2 : i1)
   hw.module private @PortNameFoo(in %clock: !seq.clock, in %1: i1, out o : i1) {
     // CHECK: hw.instance "PortNameFoo_cover"
-    // CHECK-SAME: @PortNameFoo_cover(clock: %clock: !seq.clock, port_1: %arg0: i1, port_2: %0: i1) -> ()
+    // CHECK-SAME: @PortNameFoo_cover(clock: %clock: !seq.clock, port_1: %0: i1, port_2: %1: i1) -> ()
     %0 = seq.from_clock %clock
     %2 = comb.xor %1, %1 : i1
     sv.cover.concurrent posedge %0, %1 label "cover__hello1"