From 12d168633c2a3c5558ddb290ea6a28aec4fdf40a Mon Sep 17 00:00:00 2001 From: Andrew Lenharth Date: Fri, 13 Oct 2023 14:25:00 -0700 Subject: [PATCH] [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 | 5 ++-- lib/Dialect/HW/ModuleImplementation.cpp | 28 ++++++++++++++++------- test/Dialect/Comb/canonicalization.mlir | 2 +- test/Dialect/DC/roundtrip.mlir | 22 +++++++++--------- test/Dialect/HW/basic.mlir | 10 ++++++++ test/Dialect/SV/hw-extract-test-code.mlir | 2 +- 6 files changed, 45 insertions(+), 24 deletions(-) diff --git a/lib/Dialect/HW/HWOps.cpp b/lib/Dialect/HW/HWOps.cpp index a6f0b5a3c934..ccf417791f2a 100644 --- a/lib/Dialect/HW/HWOps.cpp +++ b/lib/Dialect/HW/HWOps.cpp @@ -104,14 +104,13 @@ static void getAsmBlockArgumentNamesImpl(mlir::Region ®ion, if (region.empty()) return; // Assign port names to the bbargs. - // auto *module = region.getParentOp(); auto module = cast(region.getParentOp()); auto *block = ®ion.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); } } diff --git a/lib/Dialect/HW/ModuleImplementation.cpp b/lib/Dialect/HW/ModuleImplementation.cpp index 464e34d77128..af97055d659a 100644 --- a/lib/Dialect/HW/ModuleImplementation.cpp +++ b/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 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); diff --git a/test/Dialect/Comb/canonicalization.mlir b/test/Dialect/Comb/canonicalization.mlir index 02f5f91df736..09d90c9e507d 100644 --- a/test/Dialect/Comb/canonicalization.mlir +++ b/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 diff --git a/test/Dialect/DC/roundtrip.mlir b/test/Dialect/DC/roundtrip.mlir index e5d10d84acdf..8e2982761b63 100644 --- a/test/Dialect/DC/roundtrip.mlir +++ b/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, in %arg2 : i32) { -// CHECK-NEXT: %0 = dc.buffer[2] %arg0 : !dc.token -// CHECK-NEXT: %1 = dc.buffer[2] %arg1 [1, 2] : !dc.value -// 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 -// CHECK-NEXT: %5 = dc.to_esi %arg0 : !dc.token -// CHECK-NEXT: %6 = dc.to_esi %arg1 : !dc.value -// CHECK-NEXT: %7 = dc.from_esi %5 : -// CHECK-NEXT: %8 = dc.from_esi %6 : +// CHECK: hw.module @foo(in %0 "" : !dc.token, in %1 "" : !dc.value, in %2 "" : i32) { +// CHECK-NEXT: %3 = dc.buffer[2] %0 : !dc.token +// CHECK-NEXT: %4 = dc.buffer[2] %1 [1, 2] : !dc.value +// 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 +// CHECK-NEXT: %8 = dc.to_esi %0 : !dc.token +// CHECK-NEXT: %9 = dc.to_esi %1 : !dc.value +// CHECK-NEXT: %10 = dc.from_esi %8 : +// CHECK-NEXT: %11 = dc.from_esi %9 : // CHECK-NEXT: hw.output // CHECK-NEXT: } diff --git a/test/Dialect/HW/basic.mlir b/test/Dialect/HW/basic.mlir index b90fa5c1638d..6a03c5b8d99a 100644 --- a/test/Dialect/HW/basic.mlir +++ b/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 +} diff --git a/test/Dialect/SV/hw-extract-test-code.mlir b/test/Dialect/SV/hw-extract-test-code.mlir index b351dc384189..6599b89501e9 100644 --- a/test/Dialect/SV/hw-extract-test-code.mlir +++ b/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"