[FIRRTL] Add input and output names to DPI intrinsic (#7265)

Fix https://github.com/llvm/circt/issues/7226. This adds support for specifying input and output names. This uses `;` separated string list following the same design as `guard` parameter of assert intrinsic.

docs/Dialects/FIRRTL/FIRRTLIntrinsics.md

@@ -230,10 +230,12 @@ by another `enable` to model a default value of results.
 For clocked calls, a low enable means that its register state transfer function is
 not called. Hence their values will not be modify in that clock.
 
-| Parameter     | Type   | Description                      |
+| Parameter     | Type   | Description                                         |
-| ------------- | ------ | -------------------------------- |
+| ------------- | ------ | --------------------------------------------------- |
-| isClocked     | int    | Set 1 if the dpi call is clocked |
+| isClocked     | int    | Set 1 if the dpi call is clocked.                   |
-| functionName  | string | Specify the function name        |
+| functionName  | string | Specify the function name.                          |
+| inputNames    | string | Semicolon-delimited list of input names.  Optional. |
+| outputName    | string | Output name. Optional.                              |
 
 
 | Port              | Direction | Type     | Description                     |

include/circt/Dialect/FIRRTL/FIRRTLIntrinsics.td

@@ -214,6 +214,8 @@ def DPICallIntrinsicOp : FIRRTLOp<"int.dpi.call",
     }];
 
   let arguments = (ins StrAttr:$functionName,
+                       OptionalAttr<StrArrayAttr>:$inputNames,
+                       OptionalAttr<StrAttr>:$outputName,
                        Optional<NonConstClockType>:$clock,
                        Optional<NonConstUInt1Type>:$enable,
                        Variadic<PassiveType>:$inputs);

lib/Dialect/FIRRTL/FIRRTLIntrinsics.cpp

@@ -665,8 +665,10 @@ public:
   using IntrinsicConverter::IntrinsicConverter;
 
   bool check(GenericIntrinsic gi) override {
-    if (gi.hasNParam(2) || gi.namedIntParam("isClocked") ||
+    if (gi.hasNParam(2, 2) || gi.namedIntParam("isClocked") ||
-        gi.namedParam("functionName"))
+        gi.namedParam("functionName") ||
+        gi.namedParam("inputNames", /*optional=*/true) ||
+        gi.namedParam("outputName", /*optional=*/true))
       return true;
     auto isClocked = getIsClocked(gi);
     // If clocked, the first operand must be a clock.
@@ -683,6 +685,14 @@ public:
                PatternRewriter &rewriter) override {
     auto isClocked = getIsClocked(gi);
     auto functionName = gi.getParamValue<StringAttr>("functionName");
+    ArrayAttr inputNamesStrArray;
+    StringAttr outputStr = gi.getParamValue<StringAttr>("outputName");
+    if (auto inputNames = gi.getParamValue<StringAttr>("inputNames")) {
+      SmallVector<StringRef> inputNamesTemporary;
+      inputNames.strref().split(inputNamesTemporary, ';', /*MaxSplit=*/-1,
+                                /*KeepEmpty=*/false);
+      inputNamesStrArray = rewriter.getStrArrayAttr(inputNamesTemporary);
+    }
     // Clock and enable are optional.
     Value clock = isClocked ? adaptor.getOperands()[0] : Value();
     Value enable = adaptor.getOperands()[static_cast<size_t>(isClocked)];
@@ -691,7 +701,8 @@ public:
         adaptor.getOperands().drop_front(static_cast<size_t>(isClocked) + 1);
 
     rewriter.replaceOpWithNewOp<DPICallIntrinsicOp>(
-        gi.op, gi.op.getResultTypes(), functionName, clock, enable, inputs);
+        gi.op, gi.op.getResultTypes(), functionName, inputNamesStrArray,
+        outputStr, clock, enable, inputs);
   }
 };
 

lib/Dialect/FIRRTL/FIRRTLOps.cpp

@@ -5535,6 +5535,16 @@ static bool isTypeAllowedForDPI(Operation *op, Type type) {
 }
 
 LogicalResult DPICallIntrinsicOp::verify() {
+  if (auto inputNames = getInputNames()) {
+    if (getInputs().size() != inputNames->size())
+      return emitError() << "inputNames has " << inputNames->size()
+                         << " elements but there are " << getInputs().size()
+                         << " input arguments";
+  }
+  if (auto outputName = getOutputName())
+    if (getNumResults() == 0)
+      return emitError() << "output name is given but there is no result";
+
   auto checkType = [this](Type type) {
     return isTypeAllowedForDPI(*this, type);
   };

lib/Dialect/FIRRTL/Transforms/LowerDPI.cpp

@@ -175,6 +175,9 @@ sim::DPIFuncOp LowerDPI::getOrCreateDPIFuncDecl(DPICallIntrinsicOp op) {
   builder.setInsertionPointToStart(circuitOp.getBodyBlock());
   auto inputTypes = op.getInputs().getTypes();
   auto outputTypes = op.getResultTypes();
+  ArrayAttr inputNames = op.getInputNamesAttr();
+  StringAttr outputName = op.getOutputNameAttr();
+  assert(outputTypes.size() <= 1);
 
   SmallVector<hw::ModulePort> ports;
   ports.reserve(inputTypes.size() + outputTypes.size());
@@ -183,7 +186,8 @@ sim::DPIFuncOp LowerDPI::getOrCreateDPIFuncDecl(DPICallIntrinsicOp op) {
   for (auto [idx, inType] : llvm::enumerate(inputTypes)) {
     hw::ModulePort port;
     port.dir = hw::ModulePort::Direction::Input;
-    port.name = builder.getStringAttr(Twine("in_") + Twine(idx));
+    port.name = inputNames ? cast<StringAttr>(inputNames[idx])
+                           : builder.getStringAttr(Twine("in_") + Twine(idx));
     port.type = lowerType(inType);
     ports.push_back(port);
   }
@@ -192,7 +196,8 @@ sim::DPIFuncOp LowerDPI::getOrCreateDPIFuncDecl(DPICallIntrinsicOp op) {
   for (auto [idx, outType] : llvm::enumerate(outputTypes)) {
     hw::ModulePort port;
     port.dir = hw::ModulePort::Direction::Output;
-    port.name = builder.getStringAttr(Twine("out_") + Twine(idx));
+    port.name = outputName ? outputName
+                           : builder.getStringAttr(Twine("out_") + Twine(idx));
     port.type = lowerType(outType);
     ports.push_back(port);
   }

test/Dialect/FIRRTL/errors.mlir

@@ -2481,6 +2481,25 @@ firrtl.circuit "DPI" {
   }
 }
 
+// -----
+
+firrtl.circuit "DPI" {
+  firrtl.module @DPI(in %clock : !firrtl.clock, in %enable : !firrtl.uint<1>, in %in_0: !firrtl.uint<8>, in %in_1: !firrtl.uint) {
+    // expected-error @below {{inputNames has 0 elements but there are 1 input arguments}}
+    %0 = firrtl.int.dpi.call "clocked_result"(%in_0) clock %clock enable %enable {inputNames=[]} : (!firrtl.uint<8>) -> !firrtl.uint<8>
+  }
+}
+
+// -----
+
+firrtl.circuit "DPI" {
+  firrtl.module @DPI(in %clock : !firrtl.clock, in %enable : !firrtl.uint<1>, in %in_0: !firrtl.uint<8>, in %in_1: !firrtl.uint) {
+    // expected-error @below {{output name is given but there is no result}}
+    firrtl.int.dpi.call "clocked_result"(%in_0) clock %clock enable %enable {outputName="foo"} : (!firrtl.uint<8>) -> ()
+  }
+}
+
+
 // -----
 
 firrtl.circuit "LHSTypes" {

test/Dialect/FIRRTL/lower-dpi.mlir

@@ -4,7 +4,7 @@
 firrtl.circuit "DPI" {
   // CHECK-NEXT: sim.func.dpi private @unclocked_result(in %in_0 : i8, in %in_1 : i8, out out_0 : i8) attributes {verilogName = "unclocked_result"}
   // CHECK-NEXT: sim.func.dpi private @clocked_void(in %in_0 : i8, in %in_1 : i8) attributes {verilogName = "clocked_void"}
-  // CHECK-NEXT: sim.func.dpi private @clocked_result(in %in_0 : i8, in %in_1 : i8, out out_0 : i8) attributes {verilogName = "clocked_result"}
+  // CHECK-NEXT: sim.func.dpi private @clocked_result(in %foo : i8, in %bar : i8, out baz : i8) attributes {verilogName = "clocked_result"}
   // CHECK-LABEL: firrtl.module @DPI
   firrtl.module @DPI(in %clock: !firrtl.clock, in %enable: !firrtl.uint<1>, in %in_0: !firrtl.uint<8>, in %in_1: !firrtl.uint<8>, out %out_0: !firrtl.uint<8>, out %out_1: !firrtl.uint<8>) attributes {convention = #firrtl<convention scalarized>} {
     // CHECK-NEXT: %0 = builtin.unrealized_conversion_cast %clock : !firrtl.clock to !seq.clock
@@ -25,7 +25,7 @@ firrtl.circuit "DPI" {
     // CHECK-NEXT: %14 = builtin.unrealized_conversion_cast %13 : i8 to !firrtl.uint<8>
     // CHECK-NEXT: firrtl.matchingconnect %out_0, %5 : !firrtl.uint<8>
     // CHECK-NEXT: firrtl.matchingconnect %out_1, %14 : !firrtl.uint<8>
-    %0 = firrtl.int.dpi.call "clocked_result"(%in_0, %in_1) clock %clock enable %enable {name = "result1"} : (!firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>
+    %0 = firrtl.int.dpi.call "clocked_result"(%in_0, %in_1) clock %clock enable %enable {inputNames = ["foo", "bar"], outputName = "baz"} : (!firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>
     firrtl.int.dpi.call "clocked_void"(%in_0, %in_1) clock %clock enable %enable : (!firrtl.uint<8>, !firrtl.uint<8>) -> ()
     %1 = firrtl.int.dpi.call "unclocked_result"(%in_0, %in_1) enable %enable {name = "result2"} : (!firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>
     firrtl.matchingconnect %out_0, %0 : !firrtl.uint<8>

test/Dialect/FIRRTL/lower-intrinsics.mlir

@@ -157,8 +157,8 @@ firrtl.circuit "Foo" {
 
   // CHECK-LABEL: firrtl.module private @DPIIntrinsicTest(in %clock: !firrtl.clock, in %enable: !firrtl.uint<1>, in %in1: !firrtl.uint<8>, in %in2: !firrtl.uint<8>)
   firrtl.module private @DPIIntrinsicTest(in %clock : !firrtl.clock, in %enable : !firrtl.uint<1>, in %in1: !firrtl.uint<8>, in %in2: !firrtl.uint<8>) {
-    // CHECK-NEXT: %0 = firrtl.int.dpi.call "clocked_result"(%in1, %in2) clock %clock enable %enable : (!firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>
+    // CHECK-NEXT: %0 = firrtl.int.dpi.call "clocked_result"(%in1, %in2) clock %clock enable %enable {inputNames = ["foo", "bar"], outputName = "baz"} : (!firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>
-    %0 = firrtl.int.generic "circt_dpi_call" <isClocked: ui32 = 1, functionName: none = "clocked_result"> %clock, %enable, %in1, %in2 : (!firrtl.clock, !firrtl.uint<1>, !firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>
+    %0 = firrtl.int.generic "circt_dpi_call" <isClocked: ui32 = 1, functionName: none = "clocked_result", inputNames: none = "foo;bar", outputName: none = "baz"> %clock, %enable, %in1, %in2 : (!firrtl.clock, !firrtl.uint<1>, !firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>
     // CHECK-NEXT: firrtl.int.dpi.call "clocked_void"(%in1, %in2) clock %clock enable %enable : (!firrtl.uint<8>, !firrtl.uint<8>) -> ()
     firrtl.int.generic "circt_dpi_call" <isClocked: ui32 = 1, functionName: none = "clocked_void"> %clock, %enable, %in1, %in2 : (!firrtl.clock, !firrtl.uint<1>, !firrtl.uint<8>, !firrtl.uint<8>) -> ()
     // CHECK-NEXT:  %1 = firrtl.int.dpi.call "unclocked_result"(%in1, %in2) enable %enable : (!firrtl.uint<8>, !firrtl.uint<8>) -> !firrtl.uint<8>

test/firtool/dpi.fir

@@ -3,9 +3,9 @@
 FIRRTL version 4.0.0
 circuit DPI:
 ; CHECK-LABEL: import "DPI-C" function void clocked_result(
-; CHECK-NEXT:   input  byte in_0,
+; CHECK-NEXT:   input  byte foo,
-; CHECK-NEXT:                    in_1,
+; CHECK-NEXT:               bar,
-; CHECK-NEXT:   output byte out_0
+; CHECK-NEXT:   output byte baz
 ; CHECK-NEXT: );
 
 ; CHECK-LABEL: import "DPI-C" function void clocked_void(
@@ -46,7 +46,7 @@ circuit DPI:
     input in: UInt<8>[2]
     output out : UInt<8>[2]
 
-    node result1 = intrinsic(circt_dpi_call<isClocked = 1, functionName="clocked_result"> : UInt<8>, clock, enable, in[0], in[1])
+    node result1 = intrinsic(circt_dpi_call<isClocked = 1, functionName="clocked_result", inputNames="foo;bar", outputName="baz"> : UInt<8>, clock, enable, in[0], in[1])
     intrinsic(circt_dpi_call<isClocked = 1, functionName="clocked_void">, clock, enable, in[0], in[1])
     node result2 = intrinsic(circt_dpi_call<isClocked = 0, functionName="unclocked_result"> : UInt<8>,  enable, in[0], in[1])