[Moore] Add module and instance port support (#7112)

Add module types and support for ports to the `moore.module` and
`moore.instance` operations. Also add a `moore.output` operation as a
terminator for modules, similar to the HW dialect. Extend ImportVerilog
to generate ports.

The ports on SVModuleOp follow a similar strategy as in HWModuleOp: all
input, inout, and ref ports are passed to the module as inputs and are
carried inside the module as block arguments, while output ports are
assigned by the OutputOp terminator. The Moore dialect reuses the
`hw::ModuleType` but does not use the `inout` direction. Instead, inout
ports will be represented as inputs with a `net<T>` wrapper type, while
ref ports will be wrapped as `ref<T>`.

Instances work identically to the HW dialect: input, inout, and ref port
connections are carried as operands, while output ports are represented
as results.

This commit also adds module and instance port support to the
ImportVerilog conversion. Regular ports are mapped to corresponding
ports on the module with the appropriate direction. Multi-ports, which
are a weird quirk of SystemVerilog that allow multiple ports to be
grouped up and presented to the outside as a single named port, are
split up into individual module ports. This is necessary since this
group can contain ports of different directions.

Inside a module Slang automatically generates local net or variable
declarations for all ports. The user may specify these declarations
themselves when using non-ANSI port lists, which Slang handles properly.
ImportVerilog inserts appropriate continuous assignments to drive the
actual input port value onto the local net or variable declaration, and
to drive the local declaration's value onto the actual output port of
the module. This properly adapts from SystemVerilog's assignable and
connectable ports that feel like nets or variables, to the Moore
dialect's by-value passing of inputs and outputs.

Instances in Slang have expressions connected to their ports. Input
ports lower this expression and directly use the result as an operand.
Output ports lower this expression and drive it through a continuous
assignment inserted after the instance, with the instance's
corresponding result on the right-hand side of the assignment.

Once we have a `ref<T>` type, and later potentially also a `net<T>`
type, the port lowering shall be revisited to ensure that inout and ref
ports are mapped to net and ref types, respectively. The lowering of
expressions connected to ports requires more care to ensure that they
are appropriately lowered to lvalues or rvalues, as needed by the port
direction. A `moore.short_circuit` operation or similar would help to
connect inout ports to the local net declarations in a module, and to
capture `alias` statements.

---------

Co-authored-by: cepheus <buyun.xu@terapines.com>
Co-authored-by: Hailong Sun <hailong.sun@terapines.com>

include/circt/Dialect/Moore/MooreDialect.td

@@ -17,6 +17,7 @@ include "mlir/IR/DialectBase.td"
 
 def MooreDialect : Dialect {
   let name = "moore";
+  let cppNamespace = "::circt::moore";
 
   let summary = "Types and operations for the Moore dialect";
   let description = [{
@@ -24,7 +25,7 @@ def MooreDialect : Dialect {
     SystemVerilog-specific constructs without ambiguities and all types
     resolved.
   }];
-  let cppNamespace = "::circt::moore";
+
   let extraClassDeclaration = [{
     /// Register all Moore types.
     void registerTypes();
@@ -34,6 +35,7 @@ def MooreDialect : Dialect {
     void printType(Type, DialectAsmPrinter &) const override;
   }];
   let useDefaultTypePrinterParser = 0;
+  let dependentDialects = ["hw::HWDialect"];
 }
 
 #endif // CIRCT_DIALECT_MOORE_MOOREDIALECT

include/circt/Dialect/Moore/MooreOps.h

@@ -13,8 +13,10 @@
 #ifndef CIRCT_DIALECT_MOORE_MOOREOPS_H
 #define CIRCT_DIALECT_MOORE_MOOREOPS_H
 
+#include "circt/Dialect/HW/HWTypes.h"
 #include "circt/Dialect/Moore/MooreDialect.h"
 #include "circt/Dialect/Moore/MooreTypes.h"
+#include "mlir/Interfaces/ControlFlowInterfaces.h"
 #include "mlir/Interfaces/InferTypeOpInterface.h"
 
 #define GET_OP_CLASSES

include/circt/Dialect/Moore/MooreOps.td

@@ -15,6 +15,7 @@ include "mlir/IR/OpAsmInterface.td"
 include "mlir/IR/OpBase.td"
 include "mlir/IR/RegionKindInterface.td"
 include "mlir/IR/SymbolInterfaces.td"
+include "mlir/Interfaces/ControlFlowInterfaces.td"
 include "mlir/Interfaces/InferTypeOpInterface.td"
 include "mlir/Interfaces/SideEffectInterfaces.td"
 include "mlir/Interfaces/MemorySlotInterfaces.td"
@@ -41,8 +42,8 @@ class ResultIsSingleBitMatchingInputDomain<string result, string input> :
 def SVModuleOp : MooreOp<"module", [
   IsolatedFromAbove,
   Symbol,
-  NoTerminator,
-  SingleBlock
+  SingleBlockImplicitTerminator<"OutputOp">,
+  DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmBlockArgumentNames"]>,
 ]> {
   let summary = "A module definition";
   let description = [{
@@ -55,15 +56,43 @@ def SVModuleOp : MooreOp<"module", [
     See IEEE 1800-2017 § 3.3 "Modules" and § 23.2 "Module definitions".
   }];
 
-  let arguments = (ins SymbolNameAttr:$sym_name);
+  let arguments = (ins
+    SymbolNameAttr:$sym_name,
+    TypeAttrOf<ModuleType>:$module_type
+  );
   let regions = (region SizedRegion<1>:$bodyRegion);
-  let assemblyFormat = [{
-    $sym_name attr-dict-with-keyword $bodyRegion
+  let hasCustomAssemblyFormat = 1;
+
+  let extraClassDeclaration = [{
+    /// Return the `moore.output` op terminator of this module.
+    OutputOp getOutputOp();
+    /// Return the list of values assigned to output ports.
+    OperandRange getOutputs();
   }];
 }
 
+def OutputOp : MooreOp<"output", [
+  Terminator, HasParent<"SVModuleOp">, Pure, ReturnLike
+]> {
+  let summary = "Assign module outputs";
+  let description = [{
+    The `moore.output` operation marks the end of a `moore.module` body region
+    and specifies the values to present for the module's output ports.
+  }];
+
+  let arguments = (ins Variadic<AnyType>:$outputs);
+  let builders = [
+    OpBuilder<(ins), [{ build($_builder, $_state, std::nullopt); }]>
+  ];
+  let assemblyFormat = [{
+    attr-dict ($outputs^ `:` type($outputs))?
+  }];
+  let hasVerifier = 1;
+}
+
 def InstanceOp : MooreOp<"instance", [
-  DeclareOpInterfaceMethods<SymbolUserOpInterface>
+  DeclareOpInterfaceMethods<SymbolUserOpInterface>,
+  DeclareOpInterfaceMethods<OpAsmOpInterface, ["getAsmResultNames"]>
 ]> {
   let summary = "Create an instance of a module";
   let description = [{
@@ -72,11 +101,15 @@ def InstanceOp : MooreOp<"instance", [
     See IEEE 1800-2017 § 23.3 "Module instances".
   }];
 
-  let arguments = (ins StrAttr:$instanceName,
-                       FlatSymbolRefAttr:$moduleName);
-  let assemblyFormat = [{
-    $instanceName $moduleName attr-dict
-  }];
+  let arguments = (ins
+    StrAttr:$instanceName,
+    FlatSymbolRefAttr:$moduleName,
+    Variadic<AnyType>:$inputs,
+    StrArrayAttr:$inputNames,
+    StrArrayAttr:$outputNames
+  );
+  let results = (outs Variadic<AnyType>:$outputs);
+  let hasCustomAssemblyFormat = 1;
 }
 
 def Initial: I32EnumAttrCase<"Initial", 0, "initial">;
@@ -158,7 +191,7 @@ def VariableOp : MooreOp<"variable", [
   let arguments = (ins StrAttr:$name, Optional<UnpackedType>:$initial);
   let results = (outs Res<UnpackedType, "", [MemAlloc]>:$result);
   let assemblyFormat = [{
-    custom<ImplicitSSAName>($name) ($initial^)? attr-dict
+    `` custom<ImplicitSSAName>($name) ($initial^)? attr-dict
     `:` type($result)
   }];
 }

include/circt/Dialect/Moore/MooreTypes.td

@@ -329,6 +329,9 @@ def UnpackedUnionType : StructLikeType<
 // Constraints
 //===----------------------------------------------------------------------===//
 
+def ModuleType : MooreType<CPred<"llvm::isa<hw::ModuleType>($_self)">,
+  "module type", "hw::ModuleType">;
+
 def UnpackedType : MooreType<CPred<"llvm::isa<moore::UnpackedType>($_self)">,
   "unpacked type", "moore::UnpackedType">;
 

lib/Conversion/ImportVerilog/ImportVerilogInternals.h

@@ -25,6 +25,19 @@
 namespace circt {
 namespace ImportVerilog {
 
+/// Port lowering information.
+struct PortLowering {
+  const slang::ast::PortSymbol *
+  Location loc;
+  BlockArgument arg;
+};
+
+/// Module lowering information.
+struct ModuleLowering {
+  moore::SVModuleOp op;
+  SmallVector<PortLowering> ports;
+};
+
 /// A helper class to facilitate the conversion from a Slang AST to MLIR
 /// operations. Keeps track of the destination MLIR module, builders, and
 /// various worklists and utilities needed for conversion.
@@ -55,7 +68,7 @@ struct Context {
 
   /// Convert hierarchy and structure AST nodes to MLIR ops.
   LogicalResult convertCompilation(slang::ast::Compilation &compilation);
-  moore::SVModuleOp
+  ModuleLowering *
   convertModuleHeader(const slang::ast::InstanceBodySymbol *module);
   LogicalResult convertModuleBody(const slang::ast::InstanceBodySymbol *module);
 
@@ -78,7 +91,9 @@ struct Context {
   /// used to produce IR that follows the source file order.
   std::map<slang::SourceLocation, Operation *> orderedRootOps;
   /// How we have lowered modules to MLIR.
-  DenseMap<const slang::ast::InstanceBodySymbol *, moore::SVModuleOp> moduleOps;
+  DenseMap<const slang::ast::InstanceBodySymbol *,
+           std::unique_ptr<ModuleLowering>>
+      modules;
   /// A list of modules for which the header has been created, but the body has
   /// not been converted yet.
   std::queue<const slang::ast::InstanceBodySymbol *> moduleWorklist;

lib/Conversion/ImportVerilog/Structure.cpp

@@ -94,15 +94,110 @@ struct MemberVisitor {
   // Skip typedefs.
   LogicalResult visit(const slang::ast::TypeAliasType &) { return success(); }
 
+  // Skip ports which are already handled by the module itself.
+  LogicalResult visit(const slang::ast::PortSymbol &) { return success(); }
+  LogicalResult visit(const slang::ast::MultiPortSymbol &) { return success(); }
+
   // Handle instances.
   LogicalResult visit(const slang::ast::InstanceSymbol &instNode) {
-    auto targetModule = context.convertModuleHeader(&instNode.body);
-    if (!targetModule)
-      return failure();
+    using slang::ast::ArgumentDirection;
+    using slang::ast::AssignmentExpression;
+    using slang::ast::MultiPortSymbol;
+    using slang::ast::PortSymbol;
 
-    builder.create<moore::InstanceOp>(
-        loc, builder.getStringAttr(instNode.name),
-        FlatSymbolRefAttr::get(targetModule.getSymNameAttr()));
+    auto *moduleLowering = context.convertModuleHeader(&instNode.body);
+    if (!moduleLowering)
+      return failure();
+    auto module = moduleLowering->op;
+    auto moduleType = module.getModuleType();
+
+    // Prepare the values that are involved in port connections. This creates
+    // rvalues for input ports and appropriate lvalues for output, inout, and
+    // ref ports. We also separate multi-ports into the individual underlying
+    // ports with their corresponding connection.
+    SmallDenseMap<const PortSymbol *, Value> portValues;
+    portValues.reserve(moduleType.getNumPorts());
+
+    for (const auto *con : instNode.getPortConnections()) {
+      const auto *expr = con->getExpression();
+      if (!expr)
+        return mlir::emitError(loc)
+               << "unconnected port `" << con->port.name << "` not supported";
+
+      // Unpack the `<expr> = EmptyArgument` pattern emitted by Slang for
+      // output and inout ports.
+      if (const auto *assign = expr->as_if<AssignmentExpression>())
+        expr = &assign->left();
+
+      // Regular ports lower the connected expression to an lvalue or rvalue and
+      // either attach it to the instance as an operand (for input, inout, and
+      // ref ports), or assign an instance output to it (for output ports).
+      if (auto *port = con->port.as_if<PortSymbol>()) {
+        // TODO: Convert as rvalue for inputs, lvalue for all others.
+        auto value = context.convertExpression(*expr);
+        if (!value)
+          return failure();
+        portValues.insert({port, value});
+        continue;
+      }
+
+      // Multi-ports lower the connected expression to an lvalue and then slice
+      // it up into multiple sub-values, one for each of the ports in the
+      // multi-port.
+      if (const auto *multiPort = con->port.as_if<MultiPortSymbol>()) {
+        // TODO: Convert as lvalue.
+        auto value = context.convertExpression(*expr);
+        if (!value)
+          return failure();
+        unsigned offset = 0;
+        auto i32 = moore::IntType::getInt(context.getContext(), 32);
+        for (const auto *port : llvm::reverse(multiPort->ports)) {
+          unsigned width = port->getType().getBitWidth();
+          auto index = builder.create<moore::ConstantOp>(loc, i32, offset);
+          auto sliceType = context.convertType(port->getType());
+          if (!sliceType)
+            return failure();
+          auto slice =
+              builder.create<moore::ExtractOp>(loc, sliceType, value, index);
+          // TODO: Read to map to rvalue for input ports.
+          portValues.insert({port, slice});
+          offset += width;
+        }
+        continue;
+      }
+
+      mlir::emitError(loc) << "unsupported instance port `" << con->port.name
+                           << "` (" << slang::ast::toString(con->port.kind)
+                           << ")";
+      return failure();
+    }
+
+    // Match the module's ports up with the port values determined above.
+    SmallVector<Value> inputValues;
+    SmallVector<Value> outputValues;
+    inputValues.reserve(moduleType.getNumInputs());
+    outputValues.reserve(moduleType.getNumOutputs());
+
+    for (auto &port : moduleLowering->ports) {
+      auto value = portValues.lookup(&port.ast);
+      assert(value && "no prepared value for port");
+      if (port.ast.direction == ArgumentDirection::Out)
+        outputValues.push_back(value);
+      else
+        inputValues.push_back(value);
+    }
+
+    // Create the instance op itself.
+    auto inputNames = builder.getArrayAttr(moduleType.getInputNames());
+    auto outputNames = builder.getArrayAttr(moduleType.getOutputNames());
+    auto inst = builder.create<moore::InstanceOp>(
+        loc, moduleType.getOutputTypes(), builder.getStringAttr(instNode.name),
+        FlatSymbolRefAttr::get(module.getSymNameAttr()), inputValues,
+        inputNames, outputNames);
+
+    // Assign output values from the instance to the connected expression.
+    for (auto [lvalue, output] : llvm::zip(outputValues, inst.getOutputs()))
+      builder.create<moore::ContinuousAssignOp>(loc, lvalue, output);
 
     return success();
   }
@@ -285,7 +380,8 @@ Context::convertCompilation(slang::ast::Compilation &compilation) {
   // Prime the root definition worklist by adding all the top-level modules.
   SmallVector<const slang::ast::InstanceSymbol *> topInstances;
   for (auto *inst : root.topInstances)
-    convertModuleHeader(&inst->body);
+    if (!convertModuleHeader(&inst->body))
+      return failure();
 
   // Convert all the root module definitions.
   while (!moduleWorklist.empty()) {
@@ -302,10 +398,18 @@ Context::convertCompilation(slang::ast::Compilation &compilation) {
 /// the op to the worklist of module bodies to be lowered. This acts like a
 /// module "declaration", allowing instances to already refer to a module even
 /// before its body has been lowered.
-moore::SVModuleOp
+ModuleLowering *
 Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) {
-  if (auto op = moduleOps.lookup(module))
-    return op;
+  using slang::ast::ArgumentDirection;
+  using slang::ast::MultiPortSymbol;
+  using slang::ast::PortSymbol;
+
+  auto &slot = modules[module];
+  if (slot)
+    return slot.get();
+  slot = std::make_unique<ModuleLowering>();
+  auto &lowering = *slot;
+
   auto loc = convertLocation(module->location);
   OpBuilder::InsertionGuard g(builder);
 
@@ -314,18 +418,49 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) {
   // only minor differences in semantics.
   if (module->getDefinition().definitionKind !=
       slang::ast::DefinitionKind::Module) {
-    mlir::emitError(loc, "unsupported construct: ")
-        << module->getDefinition().getKindString();
+    mlir::emitError(loc) << "unsupported construct: "
+                         << module->getDefinition().getKindString();
     return {};
   }
 
   // Handle the port list.
+  auto block = std::make_unique<Block>();
+  SmallVector<hw::ModulePort> modulePorts;
   for (auto *symbol : module->getPortList()) {
-    auto portLoc = convertLocation(symbol->location);
-    mlir::emitError(portLoc, "unsupported module port: ")
-        << slang::ast::toString(symbol->kind);
-    return {};
+    auto handlePort = [&](const PortSymbol &port) {
+      auto portLoc = convertLocation(port.location);
+      auto type = convertType(port.getType());
+      if (!type)
+        return failure();
+      auto portName = builder.getStringAttr(port.name);
+      BlockArgument arg;
+      if (port.direction == ArgumentDirection::Out) {
+        modulePorts.push_back({portName, type, hw::ModulePort::Output});
+      } else {
+        // TODO: Once we have net/ref type wrappers, wrap `inout` and `ref`
+        // ports in the corresponding type wrapper.
+        modulePorts.push_back({portName, type, hw::ModulePort::Input});
+        arg = block->addArgument(type, portLoc);
+      }
+      lowering.ports.push_back({port, portLoc, arg});
+      return success();
+    };
+
+    if (const auto *port = symbol->as_if<PortSymbol>()) {
+      if (failed(handlePort(*port)))
+        return {};
+    } else if (const auto *multiPort = symbol->as_if<MultiPortSymbol>()) {
+      for (auto *port : multiPort->ports)
+        if (failed(handlePort(*port)))
+          return {};
+    } else {
+      mlir::emitError(convertLocation(symbol->location))
+          << "unsupported module port `" << symbol->name << "` ("
+          << slang::ast::toString(symbol->kind) << ")";
+      return {};
+    }
   }
+  auto moduleType = hw::ModuleType::get(getContext(), modulePorts);
 
   // Pick an insertion point for this module according to the source file
   // location.
@@ -336,9 +471,11 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) {
     builder.setInsertionPoint(it->second);
 
   // Create an empty module that corresponds to this module.
-  auto moduleOp = builder.create<moore::SVModuleOp>(loc, module->name);
+  auto moduleOp =
+      builder.create<moore::SVModuleOp>(loc, module->name, moduleType);
   orderedRootOps.insert(it, {module->location, moduleOp});
-  moduleOp.getBodyRegion().emplaceBlock();
+  moduleOp.getBodyRegion().push_back(block.release());
+  lowering.op = moduleOp;
 
   // Add the module to the symbol table of the MLIR module, which uniquifies its
   // name as we'd expect.
@@ -346,19 +483,18 @@ Context::convertModuleHeader(const slang::ast::InstanceBodySymbol *module) {
 
   // Schedule the body to be lowered.
   moduleWorklist.push(module);
-  moduleOps.insert({module, moduleOp});
-  return moduleOp;
+  return &lowering;
 }
 
 /// Convert a module's body to the corresponding IR ops. The module op must have
 /// already been created earlier through a `convertModuleHeader` call.
 LogicalResult
 Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) {
-  auto moduleOp = moduleOps.lookup(module);
-  assert(moduleOp);
+  auto &lowering = *modules[module];
   OpBuilder::InsertionGuard g(builder);
-  builder.setInsertionPointToEnd(moduleOp.getBody());
+  builder.setInsertionPointToEnd(lowering.op.getBody());
 
+  // Convert the body of the module.
   ValueSymbolScope scope(valueSymbols);
   for (auto &member : module->members()) {
     auto loc = convertLocation(member.location);
@@ -366,5 +502,37 @@ Context::convertModuleBody(const slang::ast::InstanceBodySymbol *module) {
       return failure();
   }
 
+  // Create additional ops to drive input port values onto the corresponding
+  // internal variables and nets, and to collect output port values for the
+  // terminator.
+  SmallVector<Value> outputs;
+  for (auto &port : lowering.ports) {
+    Value value;
+    if (auto *expr = port.ast.getInternalExpr()) {
+      // TODO: Once we have separate lvalue/rvalue lowering, this should use
+      // rvalue lowering for outputs and lvalue lowering for everything else.
+      value = convertExpression(*expr);
+    } else if (port.ast.internalSymbol) {
+      if (const auto *sym =
+              port.ast.internalSymbol->as_if<slang::ast::ValueSymbol>())
+        value = valueSymbols.lookup(sym);
+    }
+    if (!value)
+      return mlir::emitError(port.loc, "unsupported port: `")
+             << port.ast.name
+             << "` does not map to an internal symbol or expression";
+
+    // Collect output port values to be returned in the terminator.
+    if (port.ast.direction == slang::ast::ArgumentDirection::Out) {
+      outputs.push_back(value);
+      continue;
+    }
+
+    // Assign the value coming in through the port to the internal net or symbol
+    // of that port.
+    builder.create<moore::ContinuousAssignOp>(port.loc, value, port.arg);
+  }
+  builder.create<moore::OutputOp>(lowering.op.getLoc(), outputs);
+
   return success();
 }

lib/Conversion/ImportVerilog/Types.cpp

@@ -27,6 +27,10 @@ struct TypeVisitor {
   }
 
   // NOLINTBEGIN(misc-no-recursion)
+  Type visit(const slang::ast::VoidType &type) {
+    return moore::VoidType::get(context.getContext());
+  }
+
   Type visit(const slang::ast::ScalarType &type) {
     return getSimpleBitVectorType(type);
   }

lib/Dialect/Moore/CMakeLists.txt

@@ -15,6 +15,7 @@ add_circt_dialect_library(CIRCTMoore
   Support
 
   LINK_LIBS PUBLIC
+  CIRCTHW
   CIRCTSupport
   MLIRIR
   MLIRInferTypeOpInterface

lib/Dialect/Moore/MooreDialect.cpp

@@ -10,6 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "circt/Dialect/HW/HWDialect.h"
 #include "circt/Dialect/Moore/MooreOps.h"
 
 using namespace circt;

lib/Dialect/Moore/MooreOps.cpp

@@ -11,32 +11,224 @@
 //===----------------------------------------------------------------------===//
 
 #include "circt/Dialect/Moore/MooreOps.h"
+#include "circt/Dialect/HW/CustomDirectiveImpl.h"
+#include "circt/Dialect/HW/ModuleImplementation.h"
 #include "circt/Support/CustomDirectiveImpl.h"
 #include "mlir/IR/Builders.h"
+#include "llvm/ADT/SmallString.h"
 
 using namespace circt;
 using namespace circt::moore;
 
+//===----------------------------------------------------------------------===//
+// SVModuleOp
+//===----------------------------------------------------------------------===//
+
+void SVModuleOp::print(OpAsmPrinter &p) {
+  p << " ";
+  p.printSymbolName(SymbolTable::getSymbolName(*this).getValue());
+  hw::module_like_impl::printModuleSignatureNew(p, getBodyRegion(),
+                                                getModuleType(), {}, {});
+  p.printOptionalAttrDictWithKeyword(getOperation()->getAttrs(),
+                                     getAttributeNames());
+  p << " ";
+  p.printRegion(getBodyRegion(), /*printEntryBlockArgs=*/false,
+                /*printBlockTerminators=*/true);
+}
+
+ParseResult SVModuleOp::parse(OpAsmParser &parser, OperationState &result) {
+  // Parse the module name.
+  StringAttr nameAttr;
+  if (parser.parseSymbolName(nameAttr, getSymNameAttrName(result.name),
+                             result.attributes))
+    return failure();
+
+  // Parse the ports.
+  SmallVector<hw::module_like_impl::PortParse> ports;
+  TypeAttr modType;
+  if (failed(
+          hw::module_like_impl::parseModuleSignature(parser, ports, modType)))
+    return failure();
+  result.addAttribute(getModuleTypeAttrName(result.name), modType);
+
+  // Parse the attributes.
+  if (failed(parser.parseOptionalAttrDictWithKeyword(result.attributes)))
+    return failure();
+
+  // Add the entry block arguments.
+  SmallVector<OpAsmParser::Argument, 4> entryArgs;
+  for (auto &port : ports)
+    if (port.direction != hw::ModulePort::Direction::Output)
+      entryArgs.push_back(port);
+
+  // Parse the optional function body.
+  auto &bodyRegion = *result.addRegion();
+  if (parser.parseRegion(bodyRegion, entryArgs))
+    return failure();
+
+  ensureTerminator(bodyRegion, parser.getBuilder(), result.location);
+  return success();
+}
+
+void SVModuleOp::getAsmBlockArgumentNames(mlir::Region &region,
+                                          mlir::OpAsmSetValueNameFn setNameFn) {
+  if (&region != &getBodyRegion())
+    return;
+  auto moduleType = getModuleType();
+  for (auto [index, arg] : llvm::enumerate(region.front().getArguments()))
+    setNameFn(arg, moduleType.getInputNameAttr(index));
+}
+
+OutputOp SVModuleOp::getOutputOp() {
+  return cast<OutputOp>(getBody()->getTerminator());
+}
+
+OperandRange SVModuleOp::getOutputs() { return getOutputOp().getOperands(); }
+
+//===----------------------------------------------------------------------===//
+// OutputOp
+//===----------------------------------------------------------------------===//
+
+LogicalResult OutputOp::verify() {
+  auto module = getParentOp();
+
+  // Check that the number of operands matches the number of output ports.
+  auto outputTypes = module.getModuleType().getOutputTypes();
+  if (outputTypes.size() != getNumOperands())
+    return emitOpError("has ")
+           << getNumOperands() << " operands, but enclosing module @"
+           << module.getSymName() << " has " << outputTypes.size()
+           << " outputs";
+
+  // Check that the operand types match the output ports.
+  for (unsigned i = 0, e = outputTypes.size(); i != e; ++i)
+    if (outputTypes[i] != getOperand(i).getType())
+      return emitOpError() << "operand " << i << " (" << getOperand(i).getType()
+                           << ") does not match output type (" << outputTypes[i]
+                           << ") of module @" << module.getSymName();
+
+  return success();
+}
+
 //===----------------------------------------------------------------------===//
 // InstanceOp
 //===----------------------------------------------------------------------===//
 
 LogicalResult InstanceOp::verifySymbolUses(SymbolTableCollection &symbolTable) {
-  auto *module =
+  // Resolve the target symbol.
+  auto *symbol =
       symbolTable.lookupNearestSymbolFrom(*this, getModuleNameAttr());
-  if (module == nullptr)
-    return emitError("unknown symbol name '") << getModuleName() << "'";
+  if (!symbol)
+    return emitOpError("references unknown symbol @") << getModuleName();
 
-  // It must be some sort of module.
-  if (!isa<SVModuleOp>(module))
-    return emitError("symbol '")
-           << getModuleName()
-           << "' must reference a 'moore.module', but got a '"
-           << module->getName() << "' instead";
+  // Check that the symbol is a SVModuleOp.
+  auto module = dyn_cast<SVModuleOp>(symbol);
+  if (!module)
+    return emitOpError("must reference a 'moore.module', but @")
+           << getModuleName() << " is a '" << symbol->getName() << "'";
+
+  // Check that the input ports match.
+  auto moduleType = module.getModuleType();
+  auto inputTypes = moduleType.getInputTypes();
+
+  if (inputTypes.size() != getNumOperands())
+    return emitOpError("has ")
+           << getNumOperands() << " operands, but target module @"
+           << module.getSymName() << " has " << inputTypes.size() << " inputs";
+
+  for (unsigned i = 0, e = inputTypes.size(); i != e; ++i)
+    if (inputTypes[i] != getOperand(i).getType())
+      return emitOpError() << "operand " << i << " (" << getOperand(i).getType()
+                           << ") does not match input type (" << inputTypes[i]
+                           << ") of module @" << module.getSymName();
+
+  // Check that the output ports match.
+  auto outputTypes = moduleType.getOutputTypes();
+
+  if (outputTypes.size() != getNumResults())
+    return emitOpError("has ")
+           << getNumOperands() << " results, but target module @"
+           << module.getSymName() << " has " << outputTypes.size()
+           << " outputs";
+
+  for (unsigned i = 0, e = outputTypes.size(); i != e; ++i)
+    if (outputTypes[i] != getResult(i).getType())
+      return emitOpError() << "result " << i << " (" << getResult(i).getType()
+                           << ") does not match output type (" << outputTypes[i]
+                           << ") of module @" << module.getSymName();
 
   return success();
 }
 
+void InstanceOp::print(OpAsmPrinter &p) {
+  p << " ";
+  p.printAttributeWithoutType(getInstanceNameAttr());
+  p << " ";
+  p.printAttributeWithoutType(getModuleNameAttr());
+  printInputPortList(p, getOperation(), getInputs(), getInputs().getTypes(),
+                     getInputNames());
+  p << " -> ";
+  printOutputPortList(p, getOperation(), getOutputs().getTypes(),
+                      getOutputNames());
+  p.printOptionalAttrDict(getOperation()->getAttrs(), getAttributeNames());
+}
+
+ParseResult InstanceOp::parse(OpAsmParser &parser, OperationState &result) {
+  // Parse the instance name.
+  StringAttr instanceName;
+  if (parser.parseAttribute(instanceName, "instanceName", result.attributes))
+    return failure();
+
+  // Parse the module name.
+  FlatSymbolRefAttr moduleName;
+  if (parser.parseAttribute(moduleName, "moduleName", result.attributes))
+    return failure();
+
+  // Parse the input port list.
+  auto loc = parser.getCurrentLocation();
+  SmallVector<OpAsmParser::UnresolvedOperand> inputs;
+  SmallVector<Type> types;
+  ArrayAttr names;
+  if (parseInputPortList(parser, inputs, types, names))
+    return failure();
+  if (parser.resolveOperands(inputs, types, loc, result.operands))
+    return failure();
+  result.addAttribute("inputNames", names);
+
+  // Parse `->`.
+  if (parser.parseArrow())
+    return failure();
+
+  // Parse the output port list.
+  types.clear();
+  if (parseOutputPortList(parser, types, names))
+    return failure();
+  result.addAttribute("outputNames", names);
+  result.addTypes(types);
+
+  // Parse the attributes.
+  if (parser.parseOptionalAttrDict(result.attributes))
+    return failure();
+
+  return success();
+}
+
+void InstanceOp::getAsmResultNames(OpAsmSetValueNameFn setNameFn) {
+  SmallString<32> name;
+  name += getInstanceName();
+  name += '.';
+  auto baseLen = name.size();
+
+  for (auto [result, portName] :
+       llvm::zip(getOutputs(), getOutputNames().getAsRange<StringAttr>())) {
+    if (!portName || portName.empty())
+      continue;
+    name.resize(baseLen);
+    name += portName.getValue();
+    setNameFn(result, name);
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // VariableOp
 //===----------------------------------------------------------------------===//

test/Conversion/ImportVerilog/basic.sv

@@ -4,19 +4,19 @@
 // Internal issue in Slang v3 about jump depending on uninitialised value.
 // UNSUPPORTED: valgrind
 
-// CHECK-LABEL: moore.module @Empty {
+// CHECK-LABEL: moore.module @Empty() {
 // CHECK:       }
 module Empty;
   ; // empty member
 endmodule
 
-// CHECK-LABEL: moore.module @NestedA {
+// CHECK-LABEL: moore.module @NestedA() {
 // CHECK:         moore.instance "NestedB" @NestedB
 // CHECK:       }
-// CHECK-LABEL: moore.module @NestedB {
+// CHECK-LABEL: moore.module @NestedB() {
 // CHECK:         moore.instance "NestedC" @NestedC
 // CHECK:       }
-// CHECK-LABEL: moore.module @NestedC {
+// CHECK-LABEL: moore.module @NestedC() {
 // CHECK:       }
 module NestedA;
   module NestedB;
@@ -25,12 +25,12 @@ module NestedA;
   endmodule
 endmodule
 
-// CHECK-LABEL: moore.module @Child {
+// CHECK-LABEL: moore.module @Child() {
 // CHECK:       }
 module Child;
 endmodule
 
-// CHECK-LABEL: moore.module @Parent
+// CHECK-LABEL: moore.module @Parent() {
 // CHECK:         moore.instance "child" @Child
 // CHECK:       }
 module Parent;
@@ -305,7 +305,7 @@ module Statements;
   end
 endmodule
 
-// CHECK-LABEL: moore.module @Expressions {
+// CHECK-LABEL: moore.module @Expressions
 module Expressions;
   // CHECK: %a = moore.variable : i32
   // CHECK: %b = moore.variable : i32
@@ -710,7 +710,7 @@ module Expressions;
   end
 endmodule
 
-// CHECK-LABEL: moore.module @Conversion {
+// CHECK-LABEL: moore.module @Conversion
 module Conversion;
   // Implicit conversion.
   // CHECK: %a = moore.variable
@@ -738,3 +738,172 @@ module Conversion;
   // CHECK: %e = moore.variable [[TMP]]
   bit signed [18:0] e = 19'(b);
 endmodule
+
+// CHECK-LABEL: moore.module @PortsTop
+module PortsTop;
+  wire x0, y0, z0;
+  logic w0;
+  // CHECK: [[B:%.+]] = moore.instance "p0" @PortsAnsi(
+  // CHECK-SAME:   a: %x0: !moore.l1
+  // CHECK-SAME:   c: %z0: !moore.l1
+  // CHECK-SAME:   d: %w0: !moore.l1
+  // CHECK-SAME: ) -> (b: !moore.l1)
+  // CHECK-NEXT: moore.assign %y0, [[B]]
+  PortsAnsi p0(x0, y0, z0, w0);
+
+  wire x1, y1, z1;
+  logic w1;
+  // CHECK: [[B:%.+]] = moore.instance "p1" @PortsNonAnsi(
+  // CHECK-SAME:   a: %x1: !moore.l1
+  // CHECK-SAME:   c: %z1: !moore.l1
+  // CHECK-SAME:   d: %w1: !moore.l1
+  // CHECK-SAME: ) -> (b: !moore.l1)
+  // CHECK-NEXT: moore.assign %y1, [[B]]
+  PortsNonAnsi p1(x1, y1, z1, w1);
+
+  wire x2;
+  wire [1:0] y2;
+  int z2;
+  wire w2, v2;
+  // CHECK: [[B0:%.+]], [[B1:%.+]], [[B2:%.+]] = moore.instance "p2" @PortsExplicit(
+  // CHECK-SAME:   a0: %x2: !moore.l1
+  // CHECK-SAME:   a1: %y2: !moore.l2
+  // CHECK-SAME: ) -> (
+  // CHECK-SAME:   b0: !moore.i32
+  // CHECK-SAME:   b1: !moore.l1
+  // CHECK-SAME:   b2: !moore.l1
+  // CHECK-SAME: )
+  // CHECK-NEXT: moore.assign %z2, [[B0]]
+  // CHECK-NEXT: moore.assign %w2, [[B1]]
+  // CHECK-NEXT: moore.assign %v2, [[B2]]
+  PortsExplicit p2(x2, y2, z2, w2, v2);
+
+  wire x3, y3;
+  wire [2:0] z3;
+  wire [1:0] w3;
+  // CHECK: [[TMP:%.+]] = moore.constant 0 :
+  // CHECK: [[V2:%.+]] = moore.extract %z3 from [[TMP]]
+  // CHECK: [[TMP:%.+]] = moore.constant 1 :
+  // CHECK: [[V1:%.+]] = moore.extract %z3 from [[TMP]]
+  // CHECK: [[TMP:%.+]] = moore.constant 2 :
+  // CHECK: [[V0:%.+]] = moore.extract %z3 from [[TMP]]
+  // CHECK: [[TMP:%.+]] = moore.constant 0 :
+  // CHECK: [[C1:%.+]] = moore.extract %w3 from [[TMP]]
+  // CHECK: [[TMP:%.+]] = moore.constant 1 :
+  // CHECK: [[C0:%.+]] = moore.extract %w3 from [[TMP]]
+  // CHECK: [[V1_VALUE:%.+]], [[C1_VALUE:%.+]] = moore.instance "p3" @MultiPorts(
+  // CHECK-SAME:   a0: %x3: !moore.l1
+  // CHECK-SAME:   a1: %y3: !moore.l1
+  // CHECK-SAME:   v0: [[V0]]: !moore.l1
+  // CHECK-SAME:   v2: [[V2]]: !moore.l1
+  // CHECK-SAME:   c0: [[C0]]: !moore.l1
+  // CHECK-SAME: ) -> (
+  // CHECK-SAME:   v1: !moore.l1
+  // CHECK-SAME:   c1: !moore.l1
+  // CHECK-SAME: )
+  // CHECK-NEXT: moore.assign [[V1]], [[V1_VALUE]]
+  // CHECK-NEXT: moore.assign [[C1]], [[C1_VALUE]]
+  MultiPorts p3(x3, y3, z3, w3);
+endmodule
+
+// CHECK-LABEL: moore.module @PortsAnsi
+module PortsAnsi(
+  // CHECK-SAME: in %a : !moore.l1
+  input a,
+  // CHECK-SAME: out b : !moore.l1
+  output b,
+  // CHECK-SAME: in %c : !moore.l1
+  inout c,
+  // CHECK-SAME: in %d : !moore.l1
+  ref d
+);
+  // Internal nets and variables created by Slang for each port.
+  // CHECK: [[A_INT:%.+]] = moore.net name "a" wire : l1
+  // CHECK: [[B_INT:%.+]] = moore.net wire : l1
+  // CHECK: [[C_INT:%.+]] = moore.net name "c" wire : l1
+  // CHECK: [[D_INT:%.+]] = moore.variable name "d" : l1
+
+  // Mapping ports to local declarations.
+  // CHECK: moore.assign [[A_INT]], %a : l1
+  // CHECK: moore.assign [[C_INT]], %c : l1
+  // CHECK: moore.assign [[D_INT]], %d : l1
+  // CHECK: moore.output [[B_INT]] : !moore.l1
+endmodule
+
+// CHECK-LABEL: moore.module @PortsNonAnsi
+// CHECK-SAME: in %a : !moore.l1
+// CHECK-SAME: out b : !moore.l1
+// CHECK-SAME: in %c : !moore.l1
+// CHECK-SAME: in %d : !moore.l1
+module PortsNonAnsi(a, b, c, d);
+  input a;
+  output b;
+  inout c;
+  ref logic d;
+endmodule
+
+// CHECK-LABEL: moore.module @PortsExplicit
+module PortsExplicit(
+  // CHECK-SAME: in %a0 : !moore.l1
+  input .a0(x),
+  // CHECK-SAME: in %a1 : !moore.l2
+  input .a1({y, z}),
+  // CHECK-SAME: out b0 : !moore.i32
+  output .b0(42),
+  // CHECK-SAME: out b1 : !moore.l1
+  output .b1(x),
+  // CHECK-SAME: out b2 : !moore.l1
+  output .b2(y ^ z)
+);
+  logic x, y, z;
+
+  // Input mappings
+  // CHECK: moore.assign %x, %a0
+  // CHECK: [[TMP:%.+]] = moore.concat %y, %z
+  // CHECK: moore.assign [[TMP]], %a1
+
+  // Output mappings
+  // CHECK: [[B0:%.+]] = moore.constant 42
+  // CHECK: [[B2:%.+]] = moore.xor %y, %z
+  // CHECK: moore.output [[B0]], %x, [[B2]]
+endmodule
+
+// CHECK-LABEL: moore.module @MultiPorts
+module MultiPorts(
+  // CHECK-SAME: in %a0 : !moore.l1
+  .a0(u[0]),
+  // CHECK-SAME: in %a1 : !moore.l1
+  .a1(u[1]),
+  // CHECK-SAME: in %v0 : !moore.l1
+  // CHECK-SAME: out v1 : !moore.l1
+  // CHECK-SAME: in %v2 : !moore.l1
+  .b({v0, v1, v2}),
+  // CHECK-SAME: in %c0 : !moore.l1
+  // CHECK-SAME: out c1 : !moore.l1
+  {c0, c1}
+);
+  // CHECK: [[V0:%.+]] = moore.net name "v0" wire
+  // CHECK: [[V1:%.+]] = moore.net wire
+  // CHECK: [[V2:%.+]] = moore.net name "v2" wire
+  // CHECK: [[C0:%.+]] = moore.net name "c0" wire
+  // CHECK: [[C1:%.+]] = moore.net wire
+  input [1:0] u;
+  input v0;
+  output v1;
+  inout v2;
+  input c0;
+  output c1;
+
+  // CHECK: [[TMP1:%.+]] = moore.constant 0 :
+  // CHECK: [[TMP2:%.+]] = moore.extract %u from [[TMP1]]
+  // CHECK: moore.assign [[TMP2]], %a0
+
+  // CHECK: [[TMP1:%.+]] = moore.constant 1 :
+  // CHECK: [[TMP2:%.+]] = moore.extract %u from [[TMP1]]
+  // CHECK: moore.assign [[TMP2]], %a1
+
+  // CHECK: moore.assign [[V0]], %v0
+  // CHECK: moore.assign [[V2]], %v2
+  // CHECK: moore.assign [[C0]], %c0
+  // CHECK: moore.output [[V1]], [[C1]]
+endmodule

test/Conversion/ImportVerilog/errors.sv

@@ -31,13 +31,6 @@ module Foo;
   genvar a;
 endmodule
 
-// -----
-module Foo(
-  // expected-error @below {{unsupported module port}}
-  input a
-);
-endmodule
-
 // -----
 module Foo;
   // expected-error @below {{unsupported construct}}

test/Dialect/Moore/basic.mlir

@@ -1,9 +1,35 @@
 // RUN: circt-opt %s -verify-diagnostics | circt-opt -verify-diagnostics | FileCheck %s
 
-// CHECK-LABEL: moore.module @Foo
-moore.module @Foo {
-  // CHECK: moore.instance "foo" @Foo
-  moore.instance "foo" @Foo
+// CHECK-LABEL: moore.module @Empty()
+moore.module @Empty() {
+  // CHECK: moore.output
+}
+
+// CHECK-LABEL: moore.module @Ports
+moore.module @Ports(
+  // CHECK-SAME: in %a : !moore.string
+  in %a : !moore.string,
+  // CHECK-SAME: out b : !moore.string
+  out b : !moore.string,
+  // CHECK-SAME: in %c : !moore.event
+  in %c : !moore.event,
+  // CHECK-SAME: out d : !moore.event
+  out d : !moore.event
+) {
+  // CHECK: moore.output %a, %c : !moore.string, !moore.event
+  moore.output %a, %c : !moore.string, !moore.event
+}
+
+// CHECK-LABEL: moore.module @Module
+moore.module @Module() {
+  // CHECK: moore.instance "empty" @Empty() -> ()
+  moore.instance "empty" @Empty() -> ()
+
+  // CHECK: moore.instance "ports" @Ports(a: %i1: !moore.string, c: %i2: !moore.event) -> (b: !moore.string, d: !moore.event)
+  %i1 = moore.variable : !moore.string
+  %i2 = moore.variable : !moore.event
+  %o1, %o2 = moore.instance "ports" @Ports(a: %i1: !moore.string, c: %i2: !moore.event) -> (b: !moore.string, d: !moore.event)
+
   // CHECK: %v1 = moore.variable : i1
   %v1 = moore.variable : i1
   %v2 = moore.variable : i1
@@ -63,12 +89,8 @@ moore.module @Foo {
   }
 }
 
-// CHECK-LABEL: moore.module @Bar
-moore.module @Bar {
-}
-
 // CHECK-LABEL: moore.module @Expressions
-moore.module @Expressions {
+moore.module @Expressions() {
   %b1 = moore.variable : i1
   %l1 = moore.variable : l1
   %b5 = moore.variable : i5

test/Dialect/Moore/errors.mlir

@@ -1,12 +1,54 @@
 // RUN: circt-opt %s --verify-diagnostics --split-input-file
 
-func.func @Foo() {
-  return
+// expected-error @below {{references unknown symbol @doesNotExist}}
+moore.instance "b1" @doesNotExist() -> ()
+
+// -----
+// expected-error @below {{must reference a 'moore.module', but @Foo is a 'func.func'}}
+moore.instance "foo" @Foo() -> ()
+func.func @Foo() { return }
+
+// -----
+// expected-error @below {{has 0 operands, but target module @Foo has 1 inputs}}
+moore.instance "foo" @Foo() -> ()
+moore.module @Foo(in %a: !moore.i42) {}
+
+// -----
+// expected-error @below {{has 0 results, but target module @Foo has 1 outputs}}
+moore.instance "foo" @Foo() -> ()
+moore.module @Foo(out a: !moore.i42) {
+  %0 = moore.constant 42 : i42
+  moore.output %0 : !moore.i42
 }
 
-moore.module @Bar {
-  // expected-error @below {{symbol 'Foo' must reference a 'moore.module', but got a 'func.func' instead}}
-  moore.instance "foo" @Foo
+// -----
+%0 = moore.constant 42 : i32
+// expected-error @below {{operand 0 ('!moore.i32') does not match input type ('!moore.string') of module @Foo}}
+moore.instance "foo" @Foo(a: %0: !moore.i32) -> ()
+moore.module @Foo(in %a: !moore.string) {}
+
+// -----
+// expected-error @below {{result 0 ('!moore.i32') does not match output type ('!moore.i42') of module @Foo}}
+moore.instance "foo" @Foo() -> (a: !moore.i32)
+moore.module @Foo(out a: !moore.i42) {
+  %0 = moore.constant 42 : i42
+  moore.output %0 : !moore.i42
+}
+
+// -----
+
+moore.module @Foo() {
+  %0 = moore.constant 42 : i32
+  // expected-error @below {{op has 1 operands, but enclosing module @Foo has 0 outputs}}
+  moore.output %0 : !moore.i32
+}
+
+// -----
+
+moore.module @Foo(out a: !moore.string) {
+  %0 = moore.constant 42 : i32
+  // expected-error @below {{op operand 0 ('!moore.i32') does not match output type ('!moore.string') of module @Foo}}
+  moore.output %0 : !moore.i32
 }
 
 // -----